From 54281489a3949e30dfbfc7497353c962ef55ad09 Mon Sep 17 00:00:00 2001 From: stanlyzoolo <51911715+stanlyzoolo@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:00:58 +0300 Subject: [PATCH 01/13] docs: add tool overlay terminal implementation plan Embedded PTY overlay (x/vt + x/xpty) replacing the tab launcher; plan revised against the 24 findings of the automated plan review. Co-Authored-By: Claude Fable 5 --- docs/plans/20260814-tool-overlay-terminal.md | 199 +++++++++++++++++++ 1 file changed, 199 insertions(+) create mode 100644 docs/plans/20260814-tool-overlay-terminal.md diff --git a/docs/plans/20260814-tool-overlay-terminal.md b/docs/plans/20260814-tool-overlay-terminal.md new file mode 100644 index 0000000..3eab11e --- /dev/null +++ b/docs/plans/20260814-tool-overlay-terminal.md @@ -0,0 +1,199 @@ +# Tool overlay terminal: run tracked tools in an embedded PTY overlay + +## Overview + +- `enter` on the selected tool in `[1] tools` keeps opening the `modeRunInput` prompt (prefill: tool name / `lastRun`, editable), but the dispatched command now runs in an **embedded terminal overlay inside keepkit** (~70% of the screen, centered, `PlaceOverlay`-dimmed background) instead of a new terminal tab. +- One path for everything — **no TUI-vs-CLI distinction**: a TUI (vim/yazi/fzf) draws and lives in the overlay with the full keyboard proxied to it; a plain CLI prints and exits, and the overlay stays with the final screen + exit status until dismissed with `esc`. +- While the process is alive **every key (esc included) goes to the tool**; `ctrl+\` is the one reserved chord (kill). After exit, `esc` closes. This is the biggest feature of the project and **replaces the tab launcher entirely** — `internal/launcher`, the auto-fallback and `tea.ExecProcess` are deleted. + +## Context (from discovery) + +- Stack (researched 2026-08, decided): `github.com/charmbracelet/x/xpty` **v0.1.4** (tagged; unix pty via creack/pty + Windows **ConPTY**) + `github.com/charmbracelet/x/vt` (**untagged — pin the pseudo-version**; VT220+truecolor emulator: `NewEmulator(w, h)`, `Write` pty bytes in, `Render()` ANSI string out, `Resize`, `SendKey`/`SendText`, `InputPipe()`). Rejected: creack/pty direct (no Windows — ConPTY never merged), `taigrr/bubbleterm` (needs Bubble Tea v2; crib its emulator↔Model wiring only). +- x/vt has an open race issue (charmbracelet/x#879, Read/Close) and grapheme-split issue (#935) → **architecture rule: only `Update` touches the emulator's screen state**; the pty is read by one goroutine posting chunks as `tea.Msg`s — the `waitForChunkCmd` pattern from the update streamer. Note the rule sidesteps the *screen-buffer* races; the input path gets its own treatment (Task 1 decides between a key→bytes encoder — preferred, zero extra goroutines — and an `InputPipe` copy goroutine with an explicit lifecycle). +- x/vt master pulls `charmbracelet/ultraviolet` (Bubble Tea v2 rendering core) into the module graph and will likely bump `x/ansi` (pinned v0.11.6 — `ui.StripANSI` delegates to it), `x/cellbuf`, `colorprofile` — all transitive deps of lipgloss/glamour, i.e. **the dependency bump alone can move every render test**. Task 1's gate is therefore the full suite, not the new package. +- Existing anchors (verified by review): `shellCommand` (`internal/model/commands.go:117`) stays — the model builds argv with it and hands it to `term.Start`. `updateRunInput` (`internal/model/mode.go:330`) is the dispatch point to rewire; its refusal path deliberately skips the `lastRun` write (`mode.go:346`). `setStickyStatus`'s only callers are the two launch statuses (`mode.go:353`, `mode.go:371`) — it dies with them, plus `TestInFlightStatusSurvivesStaleExpiry` in `status_test.go`. `mode_test.go`'s `TestRunInputEnterStoresLastRun` asserts the post-enter mode and **will break at the switch** — it is in Task 5's file list. +- keepkit is on bubbletea v1.3.10; x/vt works in a v1 app — `Render()` returns a plain ANSI string. +- Patterns to follow: update streamer (`startUpdateCmd`/`waitForChunkCmd`, elapsed stamped in the cmd, never in `Update`), `modeAPIStatus` modality, `updateOutcomeBlock`'s helpers (`formatElapsed`, `fitCells`, `footerSep` — `render.go:2370`) for the exit line, the `restarter` narrow-interface idiom from main.go for the session seam, `toggleZoom`'s `!m.ready`-first refusal idiom. + +## Development Approach + +- **testing approach**: Regular (code first, then tests in the same task) +- complete each task fully before moving to the next +- make small, focused changes; the old launch path stays intact and green until the single switch-over task (Task 5) +- **CRITICAL: every task MUST include new/updated tests** for code changes in that task + - tests are not optional - they are a required part of the checklist + - unit tests for new and modified functions, covering success and error scenarios +- **CRITICAL: all tests must pass before starting next task** (`go test -race ./...`) - no exceptions +- **CRITICAL: update this plan file when scope changes during implementation** (➕/⚠️ prefixes) +- no meta.yaml schema changes; no config files written by this feature + +## Testing Strategy + +- **unit tests**: required for every task; the per-task gate is `go build ./...` + `go vet ./...` + `go test -race ./...` + `golangci-lint run` (full tree, not just the touched package — see the dependency-bump note above), plus `GOOS=windows go build ./...` wherever a build tag or dependency changes. Note: every symbol added in Tasks 2–4 is unreferenced from non-test code until Task 5 — **its own task's tests are what keep `unused` quiet**. +- **e2e tests**: none in this project (TUI verified by model-level tests, per existing convention); x/vt being pure Go means even alt-screen/truecolor rendering is assertable in model tests +- **real pty**: only in `internal/term`'s own tests (unix `sh -c echo …`, no network), under `-race`. `internal/term` writes no config and does no `logx` logging (failures ride `Exit.Err`), so it needs no `TestMain` seam — stated in the package doc, not left as an omission. +- **model tests never execute the cmd returned by the run-prompt enter** — it would spawn a real pty; assert on the returned cmd/state only (the `assertOnlyExpiryTick` hazard). + +## Progress Tracking + +- mark completed items with `[x]` immediately when done +- add newly discovered tasks with ➕ prefix +- document issues/blockers with ⚠️ prefix +- keep plan in sync with actual work done + +## Solution Overview + +- New package **`internal/term`** — bottom of the import graph, no TUI knowledge (the architectural slot `internal/launcher` vacates): `Session` owns the pty lifecycle. The **model builds argv with its existing `shellCommand`** and passes it to `term.Start` — `internal/term` stays goos-agnostic and needs no duplicate. `Start` creates the `xpty.Pty`, starts **one reader goroutine** pty → chunk channel; the exit (err + elapsed, stamped in the goroutine) lands as the final event on the same channel, then the channel closes. +- **`vt.Emulator` lives on `Model`, not in `internal/term`**: only `Update` writes to its screen state (`termChunkMsg` → `m.termEmu.Write`). Input: `tea.KeyMsg` → key-translation → **preferably an encoder + `Session.Write` (no extra goroutine)**; the `InputPipe()` copy goroutine is the fallback, with an explicit close-order lifecycle (Task 1 decides, Task 3 implements). +- New **`inputMode: modeToolOverlay`** — modal like `modeAPIStatus`: while the process is alive its handler forwards every key to the tool (esc included) and reserves only `ctrl+\` (kill via `Session.Kill`); after `termExitMsg` only `esc` acts (cleanup: close session, `termEmu = nil`, back to `modeNormal`). `overlayVisible()` gains this third member — **mouse gating rides along; `View()`'s fg picker does not** (it is a two-way `if` that must become a `switch m.mode`, done in Task 2 with a placeholder body). One overlay at a time is structural (the mode) — no `launchingFor`-style guard. +- Geometry (owned by `termGeometry`, which must agree with the frame arithmetic, so it lives in Task 4): the **outer block** is 70% of width/height, centered; `Styles.OverlayBorder` is a rounded border **plus `Padding(0, 1)`**, so the emulator body is `emuW = outerW - 4`, `emuH = outerH - 2 - 1 (title row) - 1 (exit row)`. The **exit row is reserved from the start** (blank while running) so the block height never changes and `PlaceOverlay` never re-centers mid-session. Min clamps ≈40×10 on the emulator body; below that — and before the first `WindowSizeMsg` (`!m.ready`, checked first per the `toggleZoom` precedent) — the keypress refuses honestly with a statusMsg. `View()` wraps the composite in `Margin(1, 0)` (the background PlaceOverlay measures is the layout, not the terminal) and PlaceOverlay clips silently past the bottom — the clamps must account for both. + +## Technical Details + +- `internal/term` API sketch (verified against the real packages as Task 1's **first** item, with a stop condition): + - `Start(shell string, args []string, w, h int, env []string) (*Session, error)` — env gets `TERM=xterm-256color`, `COLORTERM=truecolor` appended; cwd inherited + - `Events() <-chan Event` where `Event` is either `Data []byte` or the final `Exit{Err error, Elapsed time.Duration}` + - `Resize(w, h int) error`, `Write(p []byte) (int, error)`, `Kill()` + - reader posts bounded chunks (≈32 KiB buffer). **pty EOF semantics**: reading the master after child exit returns `EIO` on Linux, EOF on macOS — both are normal termination, never `Exit.Err`; the verdict comes from `cmd.Wait()`/xpty's wait alone + - `waitForTermChunkCmd` on the model side drains pending chunks non-blockingly into one msg; **a drain that reaches `Exit` stops there and delivers the accumulated data first** — the exit goes out as the next message (otherwise a short-lived CLI's final screen is lost, defeating the esc-after-exit design) +- Messages: `termStartedMsg{session}` (a start error surfaces as an immediate `termExitMsg`), `termChunkMsg{data []byte}`, `termExitMsg{err, elapsed}`. Elapsed is stamped in the session goroutine, never in `Update`. +- Model state: `m.termSession` (narrow interface, fake-able), `m.termEmu`, `m.termExit *termExitMsg` (nil while running), `m.termW/termH`, `m.termToolName`. **Pre-`termStartedMsg` window**: session and emulator are nil between dispatch and the started msg — keys are dropped, `ctrl+\` is a no-op, the overlay renders a `starting …` body (the `starting update…` idiom); a panic here would re-panic through `logx.Recover` and crash keepkit, so the guard is structural, not cosmetic. +- Rendering (`render.go`): frame `Styles.OverlayBorder`, title = tool name with a dim `ctrl+\ kill` hint on the right while running; body = `m.termEmu.Render()` (exactly w×h); the reserved bottom row carries the outcome line after exit, built from `updateOutcomeBlock`'s helpers: `✓ exited · · esc close` / `✕ exit · · esc close` / `✕ killed · · esc close` / `✕ failed to start · · esc close`. No statusMsg on exit — the screen is the answer. **The child's cursor is rendered**: reverse-video the cell at the emulator's cursor position while the process is alive (hidden after exit; honour the emulator's cursor-visibility state if exposed). +- Status bar: `renderStatusBar` gets a `modeToolOverlay` branch (its siblings all have one; without it the bar advertises six dead global keys incl. a `q quit` that cannot fire): while running ` running ctrl+\ kill`, after exit ` exited esc close` — within the no-truncate budget (`TestStatusBarNeverWraps`). +- Key translation: rune keys → text input, named keys (arrows, enter, backspace, tab, home/end, pgup/pgdn, F-keys, ctrl+letter) → key events; exact mechanism (encoder vs `SendKey`+pipe) fixed in Task 1. Mouse is **not** proxied in v1 (`SendMouse` exists — YAGNI). Child OSC title/bell events ignored in v1. +- Not-installed tool: `sh` prints `command not found` into the overlay and exits 127 — visible with the ✕ line, no special handling (replaces the old `notFoundExit` mapping). +- Kill path: **never call `proc.DetachTTY` on the pty command** — the pty's own `Setsid`+`Setctty` make the child a session leader, and `DetachTTY` would clobber `Setctty` and break the feature. Unix kill goes to the process group (negative pid, the `KillGroup` idea) off the pty-created session; on the ConPTY path verify `cmd.Process` is populated — if xpty spawns the process itself, kill through xpty's own API and record the Windows degradation beside `restart_windows`'s precedent. +- Quit-while-open is unreachable by design (all keys go to the tool): the way out is quitting the tool (or `ctrl+\`), then `esc`. If keepkit itself dies (SIGTERM/kill), the closing pty master HUPs the child — accepted. +- Windows: works via ConPTY (`xpty`); runtime untested by CI — accepted, the same level as `restart_windows`; the existing `GOOS=windows go build` cross-compile step covers the build. +- Launch during a running update stays allowed (independent concerns; the update log keeps streaming into `[3]` under the dim). +- go.mod: `x/xpty v0.1.4` (tagged) + `x/vt` pinned pseudo-version; upstream issues #879/#935 are tracked limitations. + +## What Goes Where + +- **Implementation Steps** (`[ ]` checkboxes): code, tests, docs — all inside this repo. +- **Post-Completion** (no checkboxes): manual verification in real terminals (real vim/yazi/fzf sessions, Windows smoke test), demo GIF decision — external to unit-testable code. + +## Implementation Steps + +### Task 1: dependencies + `internal/term` package — pty session with reader goroutine + +**Files:** +- Create: `internal/term/session.go` +- Create: `internal/term/session_test.go` +- Create: `docs/research/pty-stack.md` +- Modify: `go.mod`, `go.sum` + +- [ ] **first, before writing `Session`**: `go get github.com/charmbracelet/x/xpty@v0.1.4` + `github.com/charmbracelet/x/vt@latest`, pin the vt pseudo-version, and verify the researched API against the real packages (`NewEmulator`/`Render`/`SendKey`/`InputPipe`, cursor accessors, `xpty.NewPty`/`Start`/`Resize`); **check whether x/vt exposes a key→bytes encoder** (`vt.EncodeKey`-style) that would let `Update` translate keys and call `Session.Write` directly — no second goroutine, the Update-only rule becomes literally true. **Stop condition: if `Render() string`, the input mechanism or the cursor API differ materially from the sketch, stop and re-plan Tasks 2–4 before writing code**; record drift with ➕ +- [ ] record the transitive bumps the vt/xpty pull causes (`x/ansi`, `x/cellbuf`, `colorprofile`, new `ultraviolet`) with ➕; if any existing render test in `internal/model`/`internal/ui` moves under the bumped deps, fix or pin **before** Task 2 so breakage is attributed to the bump, not to later feature code +- [ ] drop the 2026-08 stack research (candidates, rejection reasons, pinned versions, upstream issues #879/#935) into `docs/research/pty-stack.md` so the pin has a rationale that outlives this plan +- [ ] implement `Session`: `Start(shell, args, w, h, env)` → xpty + command (env: `TERM=xterm-256color`, `COLORTERM=truecolor`; cwd inherited), one reader goroutine → events channel; final `Exit{Err, Elapsed}` (elapsed stamped in the goroutine) then close. **Treat `EIO`/`os.ErrClosed` on the master read as normal EOF** (Linux vs macOS differ) — the verdict comes from the wait, never from the read error +- [ ] implement `Resize`, `Write`, `Kill` — **no `proc.DetachTTY` on the pty command** (it would clobber the pty's `Setctty`); unix kill signals the pty-led process group; verify `cmd.Process` is populated on the ConPTY path, else kill via xpty's own API (record the Windows shape with ➕) +- [ ] state in the package doc: no config paths, no `logx` — failures ride `Exit.Err`; hence no `TestMain` seam +- [ ] write tests (unix): `sh -c 'printf hi'` → data chunk then `Exit{Err: nil}` with elapsed > 0 (this is also the Linux `EIO`-is-not-an-error test); non-zero exit surfaces in `Exit.Err`; `Kill` terminates a `sleep` and the channel closes (no goroutine leak); `Resize` returns no error; all under `-race` +- [ ] write error-case tests: `Start` with a bogus shell → error, no goroutine leak (channel closes) +- [ ] run the full gate: `go build ./...` + `go vet ./...` + `go test -race ./...` + `golangci-lint run` + `GOOS=windows go build ./...` - must pass before task 2 + +### Task 2: model plumbing — mode, msgs, cmds, emulator state (no dispatch change yet) + +**Files:** +- Create: `internal/model/overlay_term.go` +- Modify: `internal/model/model.go`, `internal/model/commands.go`, `internal/model/render.go` +- Modify: `internal/model/mouse_test.go`, `internal/model/zoom_test.go` +- Create: `internal/model/overlay_term_test.go` + +- [ ] add `modeToolOverlay` to the `inputMode` enum; extend `overlayVisible()` with it — mouse gating rides along, **`View()`'s fg picker does not**: turn the `if m.mode == modeHotkeys` two-way pick into a `switch m.mode` with a placeholder `modeToolOverlay` body here, so Task 4 only fills the renderer in +- [ ] add model state: `termSession` behind a narrow local interface (the `restarter` idiom — `var _ termSession = (*term.Session)(nil)`), `termEmu`, `termExit`, `termW/termH`, `termToolName` +- [ ] add msgs `termStartedMsg`/`termChunkMsg`/`termExitMsg` and cmds `startTermCmd(shell, args, w, h)` (safeCmd-wrapped) + `waitForTermChunkCmd(session)` with the non-blocking drain that **stops at `Exit` and delivers accumulated data first**; handlers in `Update`: started → create emulator (Update-only rule) + wire the input path per Task 1's decision, chain the wait cmd; chunk → `termEmu.Write` + chain; exit → store `termExit`, stop chaining +- [ ] if Task 1 landed on the `InputPipe` copy goroutine: implement its explicit lifecycle — started exactly once per session when both ends exist; close order on cleanup is `Kill`/wait → close pty → close the emulator's input pipe → goroutine returns; it must never outlive `esc` +- [ ] add `modeToolOverlay` to the modal tables in `mouse_test.go` (mouse no-op set, ~line 201) and `zoom_test.go` (modal `z` guard, ~line 287) — these tables are what "rides along" means +- [ ] write tests: handlers drive a fake session (chunk channel + recorded `Write`/`Kill`/`Resize`); chunk msg reaches a real `vt` emulator and `Render()` shows the bytes (pure Go — no pty); a fake whose channel holds data+data+Exit yields both data chunks before `termExitMsg` and `Render()` shows all of it; exit msg stores status and stops the chain; if the pipe goroutine exists — a `-race` leak test for its close order +- [ ] run the full gate (build + vet + `go test -race ./...` + lint) - must pass before task 3 + +### Task 3: input routing — `updateToolOverlay` handler and key translation + +**Files:** +- Modify: `internal/model/mode.go`, `internal/model/overlay_term.go` +- Modify: `internal/model/overlay_term_test.go` + +- [ ] implement key translation `tea.KeyMsg` → tool input (runes → text; named keys/ctrl-chords → key events; mechanism per Task 1's decision), confirmed against the pinned vt version; esc translates and is **sent**, not consumed, while the process runs +- [ ] add `case modeToolOverlay: return m.updateToolOverlay(msg)` to the mode dispatch **unwrapped** — deliberately *not* through `flushPendingLaunch` like its siblings: a deferred exec fallback must not fire `tea.ExecProcess` on the very keystroke that closes the overlay (the wrapper disappears entirely in Task 5) +- [ ] `updateToolOverlay`: process alive → translate & send everything except `ctrl+\` (→ `Session.Kill`, stays in mode until `termExitMsg`); process exited → `esc` cleans up (session close, `termEmu = nil`, `modeNormal`), everything else no-op +- [ ] **pre-`termStartedMsg` nil guard**: keys arriving before the session exists are dropped, `ctrl+\` is a no-op there (a nil deref would re-panic through `logx.Recover` and crash keepkit) +- [ ] write tests: keys (incl. esc, ctrl+c) reach the fake's input while alive; `ctrl+\` triggers `Kill` and mode holds; esc before exit does NOT close; esc after `termExitMsg` cleans and returns to `modeNormal`; non-esc after exit is a no-op; a key in the pre-started state neither panics nor reaches anything +- [ ] run the full gate (build + vet + `go test -race ./...` + lint) - must pass before task 4 + +### Task 4: rendering — geometry, overlay frame, cursor, exit line, status bar, resize + +**Files:** +- Modify: `internal/model/render.go`, `internal/model/overlay_term.go`, `internal/model/model.go` (`applyLayout`/`WindowSizeMsg`) +- Modify: `internal/model/overlay_term_test.go` (and/or `render_test.go`) + +- [ ] implement `termGeometry(width, height)` beside the frame arithmetic it must agree with: outer block = 70%×70% centered; body `emuW = outerW - 4` (border + `Padding(0,1)`), `emuH = outerH - 2 - 1 (title) - 1 (reserved exit row)`; account for `View()`'s `Margin(1, 0)` and PlaceOverlay's silent bottom clip; min clamps ≈40×10 body; ok=false below them **and when `!m.ready` (checked first, the `toggleZoom` idiom)** +- [ ] render the overlay in the `View()` switch: `OverlayBorder` frame, title = tool name + dim `ctrl+\ kill` right-hint while running; body = `termEmu.Render()` (pre-start: `starting …`); the **exit row is reserved from the start** (blank while running) so the block height is constant and the overlay never re-centers at exit +- [ ] render the child's cursor: reverse-video the cell at the emulator's cursor position while the process is alive, hidden after exit; honour the emulator's cursor-visibility state if the API exposes it +- [ ] fill the reserved row after exit via `updateOutcomeBlock`'s helpers (`formatElapsed`/`fitCells`/`footerSep`): `✓ exited · · esc close` / `✕ exit · · esc close` / `✕ killed · · esc close` / `✕ failed to start · · esc close` (`✓`/`✕` in `Ok`/`Danger`) +- [ ] add the `renderStatusBar` branch for `modeToolOverlay` (its siblings all have one — without it the bar advertises six dead global keys): running → ` running ctrl+\ kill`, exited → ` exited esc close`, within the no-truncate budget +- [ ] handle `WindowSizeMsg` while open: recompute `termGeometry`, `termEmu.Resize` + `Session.Resize`; shrinking below the minimum keeps the overlay at the clamped floor (no mid-session kill) +- [ ] write tests: rendered View contains frame, title, hint, `starting…` body pre-start, cursor cell reverse-video while alive, all four outcome lines after their exits; **block height identical before and after `termExitMsg`**; dimmed background; a body line carrying SGR renders at the right visible width inside the frame and no escape leaks past the frame's right edge (assert on `stripANSI(View())` width and the dim margins); status-bar branch in both states; resize propagates to fake session and emulator; refusal on a tiny terminal and on `!m.ready` +- [ ] run the full gate (build + vet + `go test -race ./...` + lint) - must pass before task 5 + +### Task 5: the switch — dispatch to overlay, delete the tab launcher wholesale + +**Files:** +- Modify: `internal/model/mode.go` (`updateRunInput`), `internal/model/model.go`, `internal/model/commands.go` +- Delete: `internal/launcher/` (whole package) +- Modify: `internal/model/mode_test.go`, `internal/model/status_test.go` +- Modify/Delete: `internal/model/launch_test.go` + +- [ ] rewire `updateRunInput`'s enter: **refusal first** (`termGeometry` incl. `!m.ready`) → statusMsg (`terminal too small to run a tool`-class wording) with **no `lastRun` write** — a launch that never started is not remembered (today's refusal comment, `mode.go:346`, keeps its meaning); on success: `lastRun` write, `startTermCmd`, `modeToolOverlay`; empty-input-cancels unchanged +- [ ] delete `internal/launcher`; in model: `startLaunchCmd`, `execToolCmd`, `launchDoneMsg`/`execDoneMsg`, `m.launchingFor`, `pendingLaunchName`/`Command` + `flushPendingLaunch` and all its modal-return call sites, `launchTimeout`, `launchFallbackStatus`, `notFoundExit`, the `launching…`/`tab open failed…` wordings +- [ ] delete `setStickyStatus` (its only callers were the two launch statuses) and `TestInFlightStatusSurvivesStaleExpiry`; keep `setStatus`/TTL machinery untouched +- [ ] keep `shellCommand` (now feeds the overlay dispatch; refresh its comment and the cross-reference on `updater.customPlan`) +- [ ] update `mode_test.go`: `TestRunInputEnterStoresLastRun` (post-enter mode → `modeToolOverlay`, `lastRun` still written); audit the other `modeRunInput` tests (`TestRunInputOpensPrefilled`, `TestRunInputEscCancels`, `TestRunInputBlankInputCancels`, `TestRunDuringUpdate`, `TestRunInputKeyGuard`) — prompt-opening ones stay, only dispatch-shape assertions change +- [ ] rewrite `launch_test.go` into overlay-dispatch tests: enter→prompt→overlay opens with prefill variants; empty list no-op; rename still clears `lastRun`; refusal writes no `lastRun`; **no test executes the returned cmd** (it would spawn a real pty) +- [ ] run the full gate + `GOOS=windows go build ./...` - must pass before task 6 + +### Task 6: surfaces — hotkeys overlay sweep + +**Files:** +- Modify: `internal/model/render.go` +- Modify: `internal/model/render_test.go` + +- [ ] `[?]` overlay tools group: the row is `{"enter", "run in a tab"}` (`render.go:797`) → `run in overlay`; that is +2 visible cells against the hard ≤76-col framed budget — measure after the change, and if `TestRenderHotkeysSizeBudget` breaks, shorten to `run overlay` rather than dropping a row +- [ ] verify no status-bar or footer branch references the deleted launch statuses; `[1]` footer `enter run` and the `modeRunInput` bar branch stay as-is +- [ ] sweep rendered-surface tests: hotkeys budget across all five self states, footer cells, status bar at the 80×24 baseline stay green with the new wording +- [ ] write/adjust tests for the changed `[?]` row +- [ ] run the full gate (build + vet + `go test -race ./...` + lint) - must pass before task 7 + +### Task 7: Verify acceptance criteria + +- [ ] verify all requirements from Overview: prompt kept, overlay launch, full keyboard proxy while alive, `ctrl+\` kill, esc-after-exit close, reserved exit line, cursor rendered, CLI-and-TUI single path, tab path fully gone +- [ ] verify edge cases: empty list, empty input cancel, launch during a running update, resize during session, tiny-terminal and `!m.ready` refusals, not-installed tool shows 127 in overlay, keys before `termStartedMsg` are safe +- [ ] run full test suite: `go test -race ./...` +- [ ] run `go vet ./...`, `golangci-lint run`, `GOOS=windows go build ./...`, `GOOS=darwin go build ./...` + +### Task 8: [Final] Update documentation + +**Files:** +- Create: `docs/design/tool-overlay.md` +- Modify: `CLAUDE.md`, `ARCHITECTURE.md`, `README.md`, `docs/design/updating.md`, `internal/updater/updater.go`, `internal/model/model.go` + +- [ ] create `docs/design/tool-overlay.md` (fourth deep-design doc: the Update-only emulator rule and why, the input-path decision, esc semantics, the kill chord and the no-`DetachTTY` invariant, geometry incl. the reserved exit row, the pty EOF rule, what was deleted and why) and link it from CLAUDE.md's design-docs table +- [ ] CLAUDE.md: replace the `internal/launcher` package row with `internal/term`; rewrite the **Run (`enter` in `focusTools`)** bullet to the overlay invariant summary; input-modes list (`modeToolOverlay`); `overlayVisible()` description; commands.go/mode.go file-table rows; drop `setStickyStatus` from the status-message lifecycle section; **"Three features" → four** in the design-docs preamble (line ~22) and the "never re-inline these three sections" sentence; fix the misquoted hotkeys row (`run in a tab`, not `run in tab`) while touching it +- [ ] re-anchor the **`planFor` idiom** onto a surviving example (`baseFor`/`shellCommand`): CLAUDE.md lines ~36/42/109, ARCHITECTURE.md ~348, `docs/design/updating.md` (two sites), `internal/updater/updater.go:93`; rewrite the "`execToolCmd` is the one unwrapped cmd" sentence in CLAUDE.md's logx section and ARCHITECTURE.md ~581 (every cmd is safeCmd-wrapped again); drop `launcher` from ARCHITECTURE.md ~65's bottom-leaf list; replace the "`launchDoneMsg`'s mode gate" mirror in `docs/design/updating.md` and the comment at `internal/model/model.go:839` with the surviving reason +- [ ] ARCHITECTURE.md: mermaid edge `model --> term` (drop `--> launcher`), package table row, mode count, rewrite the "Running a tool (`enter`)" section, "Three areas" → four (~20-22) +- [ ] README.md: rewrite the Features bullet at ~64-65 (the adapter list is the whole sentence and it all goes), Usage `enter` line at ~129-131, and the `[1]` hint enumeration at ~153 +- [ ] move this plan to `docs/plans/completed/` + +## Post-Completion + +**Manual verification** (real terminals, real tools — unit tests must not touch them): +- vim in the overlay: esc reaches vim (mode switch works), cursor visible and tracking, `:q` exits, ✓ line shows, esc closes, keepkit screen intact +- yazi/fzf session end-to-end; a plain `rg --version` run: output + ✓ line stays until esc (verify the final screen survived the drain) +- `ctrl+\` on a hung `sleep 1000` (✕ killed line); terminal resize mid-vim; launch while an update streams in `[3]` +- Windows smoke test via ConPTY when a Windows machine is available (CI only cross-compiles) +- truecolor/alt-screen fidelity spot-check (btop or similar) — known upstream limits (#935 graphemes) noted, not fixed here + +**External follow-ups**: +- demo GIFs (`demo/hero.gif`, `demo/update.gif`) show the old launch flow — decide on regeneration via the demo-gifs skill after merge +- track upstream x/vt issues #879/#935; a future tagged x/vt release replaces the pinned pseudo-version From 9795ccf9cb9dd196134c3120efd1f95c7d2407f6 Mon Sep 17 00:00:00 2001 From: stanlyzoolo <51911715+stanlyzoolo@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:22:37 +0300 Subject: [PATCH 02/13] feat: add internal/term, a pty session for the tool overlay Bottom-of-the-graph package with no TUI knowledge: a Session owns one pty and one reader goroutine, and everything it observes leaves through Events() as a value the model can consume with the update streamer's waitForChunkCmd pattern. Three things the plan assumed turned out otherwise, all recorded in docs/research/pty-stack.md so the pins outlive the plan: - there is no key encoder in the stack, and vt.SendKey encodes against DECCKM, which no accessor exposes - so a hand-rolled encoder would send the wrong arrows to exactly the full-screen tools this feature is for. - Emulator.Close() is upstream race x#879, reproduced here under -race. Closing InputPipe()'s writer stops a parked reader without touching the unsynchronised bool. - xpty sets neither Setsid nor Setctty, so term sets them itself. The no-DetachTTY invariant survives with a sharper reason: it would assign SysProcAttr wholesale and drop them. The dependency pull bumps go-runewidth and displaywidth, which keepkit measures glyph widths with; the full suite was run on the bump alone before any feature code and stayed green. Co-Authored-By: Claude Opus 5 (1M context) --- docs/plans/20260814-tool-overlay-terminal.md | 33 +- docs/research/pty-stack.md | 166 ++++++++++ go.mod | 24 +- go.sum | 48 ++- internal/term/attr_unix.go | 19 ++ internal/term/attr_windows.go | 11 + internal/term/session.go | 171 ++++++++++ internal/term/session_test.go | 316 +++++++++++++++++++ 8 files changed, 753 insertions(+), 35 deletions(-) create mode 100644 docs/research/pty-stack.md create mode 100644 internal/term/attr_unix.go create mode 100644 internal/term/attr_windows.go create mode 100644 internal/term/session.go create mode 100644 internal/term/session_test.go diff --git a/docs/plans/20260814-tool-overlay-terminal.md b/docs/plans/20260814-tool-overlay-terminal.md index 3eab11e..c824254 100644 --- a/docs/plans/20260814-tool-overlay-terminal.md +++ b/docs/plans/20260814-tool-overlay-terminal.md @@ -44,7 +44,7 @@ ## Solution Overview - New package **`internal/term`** — bottom of the import graph, no TUI knowledge (the architectural slot `internal/launcher` vacates): `Session` owns the pty lifecycle. The **model builds argv with its existing `shellCommand`** and passes it to `term.Start` — `internal/term` stays goos-agnostic and needs no duplicate. `Start` creates the `xpty.Pty`, starts **one reader goroutine** pty → chunk channel; the exit (err + elapsed, stamped in the goroutine) lands as the final event on the same channel, then the channel closes. -- **`vt.Emulator` lives on `Model`, not in `internal/term`**: only `Update` writes to its screen state (`termChunkMsg` → `m.termEmu.Write`). Input: `tea.KeyMsg` → key-translation → **preferably an encoder + `Session.Write` (no extra goroutine)**; the `InputPipe()` copy goroutine is the fallback, with an explicit close-order lifecycle (Task 1 decides, Task 3 implements). +- **`vt.Emulator` lives on `Model`, not in `internal/term`**: only `Update` writes to its screen state (`termChunkMsg` → `m.termEmu.Write`). Input: ⚠️ *decided in Task 1* — the preferred encoder **does not exist** in the pinned stack and could not be written correctly (DECCKM is unexported), so this is the **copy goroutine**: `Update` calls `emu.SendKey`/`SendText`, a per-session goroutine relays `emu.Read` → `Session.Write`, and teardown closes **`InputPipe()`'s writer** (never `Emulator.Close()`, which is upstream race #879). - New **`inputMode: modeToolOverlay`** — modal like `modeAPIStatus`: while the process is alive its handler forwards every key to the tool (esc included) and reserves only `ctrl+\` (kill via `Session.Kill`); after `termExitMsg` only `esc` acts (cleanup: close session, `termEmu = nil`, back to `modeNormal`). `overlayVisible()` gains this third member — **mouse gating rides along; `View()`'s fg picker does not** (it is a two-way `if` that must become a `switch m.mode`, done in Task 2 with a placeholder body). One overlay at a time is structural (the mode) — no `launchingFor`-style guard. - Geometry (owned by `termGeometry`, which must agree with the frame arithmetic, so it lives in Task 4): the **outer block** is 70% of width/height, centered; `Styles.OverlayBorder` is a rounded border **plus `Padding(0, 1)`**, so the emulator body is `emuW = outerW - 4`, `emuH = outerH - 2 - 1 (title row) - 1 (exit row)`. The **exit row is reserved from the start** (blank while running) so the block height never changes and `PlaceOverlay` never re-centers mid-session. Min clamps ≈40×10 on the emulator body; below that — and before the first `WindowSizeMsg` (`!m.ready`, checked first per the `toggleZoom` precedent) — the keypress refuses honestly with a statusMsg. `View()` wraps the composite in `Margin(1, 0)` (the background PlaceOverlay measures is the layout, not the terminal) and PlaceOverlay clips silently past the bottom — the clamps must account for both. @@ -62,7 +62,7 @@ - Status bar: `renderStatusBar` gets a `modeToolOverlay` branch (its siblings all have one; without it the bar advertises six dead global keys incl. a `q quit` that cannot fire): while running ` running ctrl+\ kill`, after exit ` exited esc close` — within the no-truncate budget (`TestStatusBarNeverWraps`). - Key translation: rune keys → text input, named keys (arrows, enter, backspace, tab, home/end, pgup/pgdn, F-keys, ctrl+letter) → key events; exact mechanism (encoder vs `SendKey`+pipe) fixed in Task 1. Mouse is **not** proxied in v1 (`SendMouse` exists — YAGNI). Child OSC title/bell events ignored in v1. - Not-installed tool: `sh` prints `command not found` into the overlay and exits 127 — visible with the ✕ line, no special handling (replaces the old `notFoundExit` mapping). -- Kill path: **never call `proc.DetachTTY` on the pty command** — the pty's own `Setsid`+`Setctty` make the child a session leader, and `DetachTTY` would clobber `Setctty` and break the feature. Unix kill goes to the process group (negative pid, the `KillGroup` idea) off the pty-created session; on the ConPTY path verify `cmd.Process` is populated — if xpty spawns the process itself, kill through xpty's own API and record the Windows degradation beside `restart_windows`'s precedent. +- Kill path: **never call `proc.DetachTTY` on the pty command** — ⚠️ *corrected in Task 1*: xpty sets neither `Setsid` nor `Setctty`, so `internal/term` sets them itself and `DetachTTY` would assign `SysProcAttr` wholesale and drop them. Unix kill goes to the process group (negative pid) off that session, via the existing `proc.KillGroup`; on the ConPTY path `cmd.Process` **is** populated (`os.FindProcess` in `ConPty.Start`), so `KillGroup`'s Windows branch needs no change. - Quit-while-open is unreachable by design (all keys go to the tool): the way out is quitting the tool (or `ctrl+\`), then `esc`. If keepkit itself dies (SIGTERM/kill), the closing pty master HUPs the child — accepted. - Windows: works via ConPTY (`xpty`); runtime untested by CI — accepted, the same level as `restart_windows`; the existing `GOOS=windows go build` cross-compile step covers the build. - Launch during a running update stays allowed (independent concerns; the update log keeps streaming into `[3]` under the dim). @@ -83,15 +83,26 @@ - Create: `docs/research/pty-stack.md` - Modify: `go.mod`, `go.sum` -- [ ] **first, before writing `Session`**: `go get github.com/charmbracelet/x/xpty@v0.1.4` + `github.com/charmbracelet/x/vt@latest`, pin the vt pseudo-version, and verify the researched API against the real packages (`NewEmulator`/`Render`/`SendKey`/`InputPipe`, cursor accessors, `xpty.NewPty`/`Start`/`Resize`); **check whether x/vt exposes a key→bytes encoder** (`vt.EncodeKey`-style) that would let `Update` translate keys and call `Session.Write` directly — no second goroutine, the Update-only rule becomes literally true. **Stop condition: if `Render() string`, the input mechanism or the cursor API differ materially from the sketch, stop and re-plan Tasks 2–4 before writing code**; record drift with ➕ -- [ ] record the transitive bumps the vt/xpty pull causes (`x/ansi`, `x/cellbuf`, `colorprofile`, new `ultraviolet`) with ➕; if any existing render test in `internal/model`/`internal/ui` moves under the bumped deps, fix or pin **before** Task 2 so breakage is attributed to the bump, not to later feature code -- [ ] drop the 2026-08 stack research (candidates, rejection reasons, pinned versions, upstream issues #879/#935) into `docs/research/pty-stack.md` so the pin has a rationale that outlives this plan -- [ ] implement `Session`: `Start(shell, args, w, h, env)` → xpty + command (env: `TERM=xterm-256color`, `COLORTERM=truecolor`; cwd inherited), one reader goroutine → events channel; final `Exit{Err, Elapsed}` (elapsed stamped in the goroutine) then close. **Treat `EIO`/`os.ErrClosed` on the master read as normal EOF** (Linux vs macOS differ) — the verdict comes from the wait, never from the read error -- [ ] implement `Resize`, `Write`, `Kill` — **no `proc.DetachTTY` on the pty command** (it would clobber the pty's `Setctty`); unix kill signals the pty-led process group; verify `cmd.Process` is populated on the ConPTY path, else kill via xpty's own API (record the Windows shape with ➕) -- [ ] state in the package doc: no config paths, no `logx` — failures ride `Exit.Err`; hence no `TestMain` seam -- [ ] write tests (unix): `sh -c 'printf hi'` → data chunk then `Exit{Err: nil}` with elapsed > 0 (this is also the Linux `EIO`-is-not-an-error test); non-zero exit surfaces in `Exit.Err`; `Kill` terminates a `sleep` and the channel closes (no goroutine leak); `Resize` returns no error; all under `-race` -- [ ] write error-case tests: `Start` with a bogus shell → error, no goroutine leak (channel closes) -- [ ] run the full gate: `go build ./...` + `go vet ./...` + `go test -race ./...` + `golangci-lint run` + `GOOS=windows go build ./...` - must pass before task 2 +- [x] **first, before writing `Session`**: `go get github.com/charmbracelet/x/xpty@v0.1.4` + `github.com/charmbracelet/x/vt@latest`, pin the vt pseudo-version, and verify the researched API against the real packages (`NewEmulator`/`Render`/`SendKey`/`InputPipe`, cursor accessors, `xpty.NewPty`/`Start`/`Resize`); **check whether x/vt exposes a key→bytes encoder** (`vt.EncodeKey`-style) that would let `Update` translate keys and call `Session.Write` directly — no second goroutine, the Update-only rule becomes literally true. **Stop condition: if `Render() string`, the input mechanism or the cursor API differ materially from the sketch, stop and re-plan Tasks 2–4 before writing code**; record drift with ➕ + - vt pinned at `v0.0.0-20260813141921-f091cedeaf78`. `Render() string`, `NewEmulator`, `Resize`, `CursorPosition()`, `IsAltScreen()`, `xpty.NewPty`/`Start`/`Resize` all match the sketch — **stop condition not tripped**, Tasks 2–4 stand. + - ➕ **drift 1 — there is no key encoder.** `ultraviolet` exports output-side `Encode*` only; `x/ansi` has none. Worse, `vt.SendKey` encodes against `isModeSet(ansi.ModeCursorKeys)` (DECCKM — the mode vim/less set, where arrows are `\x1bOA` not `\x1b[A`) and **no accessor exposes it**, so a hand-rolled encoder would be wrong for exactly the full-screen tools this feature exists for. → **Task 3 takes the plan's fallback: `SendKey` + a copy goroutine.** + - ➕ **drift 2 — the `Emulator.Close()` teardown is the upstream race.** #879 reproduced here under `-race` (`e.closed` written by `Close`, read by every `Read`). **Workaround: never call `Emulator.Close()`** — `InputPipe()` returns the `*io.PipeWriter`; closing *that* unblocks the parked `Read` through `io.Pipe`'s own synchronisation and never touches the bool. Verified race-clean; pinned by `TestSessionInputPipeCloseIsRaceFree`, with a fallback to `Close()` if the assertion ever fails. + - ➕ **drift 3 — `xpty` does *not* make the child a session leader.** The plan assumed "the pty's own `Setsid`+`Setctty`"; `UnixPty.Start` only wires stdio and calls `cmd.Start()`. `internal/term` sets `SysProcAttr{Setsid: true, Setctty: true}` itself (unix build). The **no-`DetachTTY` invariant survives with a sharper reason**: it assigns `SysProcAttr` wholesale and would drop our `Setctty`. + - ➕ **Windows kill needs no detour**: `ConPty.Start` populates `cmd.Process` via `os.FindProcess`, so `proc.KillGroup`'s existing Windows branch works as-is. +- [x] record the transitive bumps the vt/xpty pull causes (`x/ansi`, `x/cellbuf`, `colorprofile`, new `ultraviolet`) with ➕; if any existing render test in `internal/model`/`internal/ui` moves under the bumped deps, fix or pin **before** Task 2 so breakage is attributed to the bump, not to later feature code + - ➕ bumps: `x/ansi` 0.11.6→0.11.7, **`go-runewidth` 0.0.19→0.0.23**, **`displaywidth` 0.9.0→0.11.0**, `uax29/v2` 2.5.0→2.7.0, `go-colorful` 1.3.0→1.4.0, `colorprofile` 0.4.1→0.4.2, `x/sys` 0.38→0.47, `x/sync` 0.17→0.19, new `ultraviolet`. `x/cellbuf` unchanged. **Full suite run on the bump alone before any feature code: all green** — no render test moved. +- [x] drop the 2026-08 stack research (candidates, rejection reasons, pinned versions, upstream issues #879/#935) into `docs/research/pty-stack.md` so the pin has a rationale that outlives this plan +- [x] implement `Session`: `Start(shell, args, w, h, env)` → xpty + command (env: `TERM=xterm-256color`, `COLORTERM=truecolor`; cwd inherited), one reader goroutine → events channel; final `Exit{Err, Elapsed}` (elapsed stamped in the goroutine) then close. **Treat `EIO`/`os.ErrClosed` on the master read as normal EOF** (Linux vs macOS differ) — the verdict comes from the wait, never from the read error + - ➕ `Exit` carries a third field, **`Killed bool`**: the session is the only place that knows both the kill and the status, so it answers rather than making the model correlate its own keypress against `signal: killed`. This is what feeds the `✕ killed` outcome line in Task 4. + - ➕ the events channel is **buffered (64)**, so the reader stays ahead of a consumer that drains once per Bubble Tea message; past it the reader blocks and throttles the child, which is the correct back-pressure. +- [x] implement `Resize`, `Write`, `Kill` — **no `proc.DetachTTY` on the pty command** (it would clobber the pty's `Setctty`); unix kill signals the pty-led process group; verify `cmd.Process` is populated on the ConPTY path, else kill via xpty's own API (record the Windows shape with ➕) + - `Kill` reuses the existing **`proc.KillGroup`** (negative pid on unix, `Process.Kill` on Windows) rather than a duplicate — `proc` is a stdlib-only bottom leaf, so `term` may import it. +- [x] state in the package doc: no config paths, no `logx` — failures ride `Exit.Err`; hence no `TestMain` seam +- [x] write tests (unix): `sh -c 'printf hi'` → data chunk then `Exit{Err: nil}` with elapsed > 0 (this is also the Linux `EIO`-is-not-an-error test); non-zero exit surfaces in `Exit.Err`; `Kill` terminates a `sleep` and the channel closes (no goroutine leak); `Resize` returns no error; all under `-race` + - ➕ four tests beyond the list, each pinning something the plan relies on elsewhere: `TestSessionKillReachesGrandchildren` (the `Setsid` premise — an `sh -c` background job must die too, or the pty never EOFs), `TestSessionEnvOverridesTerm` (os/exec's last-duplicate-wins is why appending TERM is enough), `TestSessionNotInstalledToolExits127` (replaces the deleted `notFoundExit` mapping), `TestSessionOutputRendersInEmulator` (the term↔model contract, so Task 2's fake session is honest). +- [x] write error-case tests: `Start` with a bogus shell → error, no goroutine leak (channel closes) +- [x] run the full gate: `go build ./...` + `go vet ./...` + `go test -race ./...` + `golangci-lint run` + `GOOS=windows go build ./...` - must pass before task 2 + - ⚠️ **local-only**: `go build` fails in a git *worktree* with `error obtaining VCS status: exit status 128` (every git command it shells out to works standalone). CI clones normally and is unaffected; locally the gate runs `go build -buildvcs=false ./...`. ### Task 2: model plumbing — mode, msgs, cmds, emulator state (no dispatch change yet) diff --git a/docs/research/pty-stack.md b/docs/research/pty-stack.md new file mode 100644 index 0000000..02cc6a7 --- /dev/null +++ b/docs/research/pty-stack.md @@ -0,0 +1,166 @@ +# The pty stack behind the tool overlay (researched 2026-08) + +Why keepkit runs a tracked tool on an embedded pseudo-terminal the way it does, +which libraries were considered, and what each pinned version is buying. This +outlives the implementation plan on purpose: the pins below are load-bearing and +a future reader upgrading them needs the reasons, not just the numbers. + +## What the feature needs + +Running `vim`, `yazi` or `fzf` *inside* keepkit's own screen means three things +at once, and no single library gives all three: + +1. a **pty** — the tool must believe it owns a terminal, or it will not draw at + all (`isatty` fails, ncurses/crossterm refuse to start); +2. a **terminal emulator** — something has to interpret the escape sequences the + tool writes and hand keepkit a rectangle of styled cells it can paint into a + Bubble Tea `View()`; +3. **Windows support**, because keepkit ships a Windows binary and its release + workflow cross-compiles for it. + +## Chosen: `charmbracelet/x/xpty` + `charmbracelet/x/vt` + +| Module | Version | Why | +|---|---|---| +| `github.com/charmbracelet/x/xpty` | **v0.1.4** (tagged) | one `Pty` interface over unix ptys (via `creack/pty`) and Windows **ConPTY**; `WaitProcess` papers over the Go runtime's inability to `cmd.Wait()` a ConPTY child | +| `github.com/charmbracelet/x/vt` | **pseudo-version `v0.0.0-20260813141921-f091cedeaf78`** — untagged upstream | VT220 + truecolor emulator in pure Go: `NewEmulator(w, h)`, `Write` pty bytes in, `Render() string` out | + +`vt` has **no tagged release**, so the pseudo-version is deliberate rather than +sloppy. Re-pin it only together with a run of the full suite — see *Dependency +blast radius* below. + +### Rejected alternatives + +- **`creack/pty` directly** — no Windows. ConPTY support has been proposed + upstream for years and never merged, so choosing it would mean shipping a + feature that silently does not exist on one of keepkit's three platforms. +- **`taigrr/bubbleterm`** — the closest thing to a drop-in, but it requires + Bubble Tea **v2** and keepkit is on v1.3.10. Upgrading the whole TUI to v2 to + get one feature is the tail wagging the dog. Its emulator↔`Model` wiring was + read for ideas; nothing was vendored. + +`vt` works fine under Bubble Tea v1 because `Render()` returns a plain ANSI +string — it needs no v2 rendering core to hand keepkit its screen. + +## What the API actually looks like (verified, not assumed) + +The implementation plan sketched this API from research; every item below was +re-checked against the pinned modules before a line of `internal/term` was +written, because the plan carried a stop condition for material drift. + +Matches the sketch: + +- `vt.NewEmulator(w, h) *Emulator`, `Write([]byte)`, `Render() string`, + `Resize(w, h)` (no error), `CursorPosition() uv.Position`, `IsAltScreen()`. +- `xpty.NewPty(w, h)`, `Pty.Start(*exec.Cmd)`, `Resize`, `Read`/`Write`/`Close`. +- `xpty.WaitProcess(ctx, cmd)` — falls back to `cmd.Wait()` off Windows and + synthesises the `*exec.ExitError` that `os.Process.Wait` fails to produce on + ConPTY, so the exit code has the same shape on every platform. +- `ConPty.Start` populates `cmd.Process` (via `os.FindProcess`), so the kill + path needs no Windows-specific detour. + +### Drift #1 — there is no key encoder, and the modes it would need are unexported + +The plan preferred translating a `tea.KeyMsg` into bytes and calling +`Session.Write` directly, because that needs no second goroutine and makes +"only `Update` touches the emulator" literally true. **That function does not +exist** in the pinned stack: `ultraviolet` exports only output-side encoders +(`EncodeCursorStyle`, `EncodeMouseMode`, …), and `x/ansi` has none either. + +Writing our own was rejected on correctness, not effort. `vt`'s `SendKey` +encodes **against emulator state**: + +```go +ack := e.isModeSet(ansi.ModeCursorKeys) // DECCKM - arrows are \x1bOA, not \x1b[A +akk := e.isModeSet(ansi.ModeNumericKeypad) // DECNKM +``` + +`isModeSet` is unexported and no accessor surfaces it, so a hand-rolled encoder +could not know whether the child had switched to application cursor keys — the +mode `vim` and `less` set on entry. Arrows in vim are not a corner case for the +project's flagship feature. So keepkit uses `vt`'s own `SendKey`, which means +accepting the copy goroutine the plan listed as the fallback. + +### Drift #2 — `xpty` does **not** make the child a session leader + +The plan assumed "the pty's own `Setsid`+`Setctty`". It has neither: +`UnixPty.Start` wires `Stdin`/`Stdout`/`Stderr` to the slave and calls +`cmd.Start()`, and that is all. Without `Setsid`+`Setctty` the child inherits +keepkit's controlling terminal, job control never engages, and a +`SIGKILL`-to-the-process-group teardown would signal the wrong group. + +`internal/term` therefore sets `SysProcAttr{Setsid: true, Setctty: true}` +itself, on the unix build only. `Ctty` defaults to 0, which is the child's +stdin — the slave that `xpty` just attached, so the default is the correct fd. + +The invariant the plan derived from its wrong premise survives intact, and now +has a sharper reason: **never call `proc.DetachTTY` on the pty command.** It +would overwrite `SysProcAttr` wholesale, dropping `Setctty` and handing the +child a pty with no controlling terminal — the exact opposite of the point. + +## Upstream issues we are living with + +### charmbracelet/x#879 — `Emulator.Read`/`Close` data race + +Real, reproduced here under `-race`, and it dictates the teardown: + +```go +func (e *Emulator) Read(p []byte) (int, error) { + if e.closed { return 0, io.EOF } // unsynchronised read + return e.pr.Read(p) +} + +func (e *Emulator) Close() error { + e.closed = true // unsynchronised write + return e.pw.CloseWithError(io.EOF) +} +``` + +A copy goroutine parked in `Read` plus a `Close()` from `Update` is a textbook +race, and `go test -race` reports it immediately. keepkit's suite runs `-race`, +so this is not a theoretical concern. + +**The workaround: never call `Emulator.Close()`.** `InputPipe()` returns the +emulator's `*io.PipeWriter`; closing *that* unblocks the parked `Read` with +`io.EOF` through `io.Pipe`'s own synchronisation and never touches the +`e.closed` bool. Verified race-clean, and `TestSessionInputPipeCloseIsRaceFree` +in `internal/term` is what keeps the knowledge from being lost the next time +somebody reaches for the obvious `Close()`. + +If a future `vt` fixes #879, the type assertion can go and `Close()` can come +back — the assertion falls back to `Close()` already if `InputPipe()` ever stops +returning an `io.Closer`. + +### charmbracelet/x#935 — grapheme splitting + +Wide graphemes split across two `Write` calls can render wrong. Not worked +around; the pty reader delivers large chunks and the failure is cosmetic and +transient. Tracked, not fixed here. + +## Dependency blast radius + +Pulling `vt` drags in `charmbracelet/ultraviolet` (the Bubble Tea v2 rendering +core) and bumps several modules that lipgloss and glamour already sat on — i.e. +**the dependency bump alone can move keepkit's render tests**, which is why the +gate for the dependency step was the full suite rather than the new package. + +Measured on the bump (`go test ./...` before any feature code): **everything +stayed green.** + +| Module | Before | After | +|---|---|---| +| `x/ansi` | v0.11.6 | v0.11.7 | +| `mattn/go-runewidth` | v0.0.19 | v0.0.23 | +| `clipperhouse/displaywidth` | v0.9.0 | v0.11.0 | +| `clipperhouse/uax29/v2` | v2.5.0 | v2.7.0 | +| `lucasb-eyer/go-colorful` | v1.3.0 | v1.4.0 | +| `charmbracelet/colorprofile` | v0.4.1 | v0.4.2 | +| `golang.org/x/sys` | v0.38.0 | v0.47.0 | +| `golang.org/x/sync` | v0.17.0 | v0.19.0 | +| `charmbracelet/ultraviolet` | — | v0.0.0-20260303162955-0b88c25f3fff (new) | + +`go-runewidth` and `displaywidth` are the two to watch on any future re-pin: +keepkit measures glyph widths in half a dozen places (the gauge, the language +band, the list markers, `insetPanelTitle`'s border arithmetic) and pins several +of them with dedicated tests precisely because a width change is invisible until +a border tears. diff --git a/go.mod b/go.mod index 3afa3ee..f564b83 100644 --- a/go.mod +++ b/go.mod @@ -7,8 +7,11 @@ require ( github.com/charmbracelet/bubbletea v1.3.10 github.com/charmbracelet/glamour v1.0.0 github.com/charmbracelet/lipgloss v1.1.1-0.20250404203927-76690c660834 - github.com/charmbracelet/x/ansi v0.11.6 - github.com/mattn/go-runewidth v0.0.19 + github.com/charmbracelet/ultraviolet v0.0.0-20260303162955-0b88c25f3fff + github.com/charmbracelet/x/ansi v0.11.7 + github.com/charmbracelet/x/vt v0.0.0-20260813141921-f091cedeaf78 + github.com/charmbracelet/x/xpty v0.1.4 + github.com/mattn/go-runewidth v0.0.23 github.com/muesli/termenv v0.16.0 github.com/yuin/goldmark-emoji v1.0.6 golang.org/x/mod v0.37.0 @@ -20,17 +23,21 @@ require ( github.com/atotto/clipboard v0.1.4 // indirect github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect github.com/aymerick/douceur v0.2.0 // indirect - github.com/charmbracelet/colorprofile v0.4.1 // indirect + github.com/charmbracelet/colorprofile v0.4.2 // indirect github.com/charmbracelet/x/cellbuf v0.0.15 // indirect + github.com/charmbracelet/x/conpty v0.2.0 // indirect + github.com/charmbracelet/x/exp/ordered v0.1.0 // indirect github.com/charmbracelet/x/exp/slice v0.0.0-20250327172914-2fdc97757edf // indirect github.com/charmbracelet/x/term v0.2.2 // indirect - github.com/clipperhouse/displaywidth v0.9.0 // indirect - github.com/clipperhouse/stringish v0.1.1 // indirect - github.com/clipperhouse/uax29/v2 v2.5.0 // indirect + github.com/charmbracelet/x/termios v0.1.1 // indirect + github.com/charmbracelet/x/windows v0.2.2 // indirect + github.com/clipperhouse/displaywidth v0.11.0 // indirect + github.com/clipperhouse/uax29/v2 v2.7.0 // indirect + github.com/creack/pty v1.1.24 // indirect github.com/dlclark/regexp2 v1.11.5 // indirect github.com/erikgeiser/coninput v0.0.0-20211004153227-1c3628e74d0f // indirect github.com/gorilla/css v1.0.1 // indirect - github.com/lucasb-eyer/go-colorful v1.3.0 // indirect + github.com/lucasb-eyer/go-colorful v1.4.0 // indirect github.com/mattn/go-isatty v0.0.20 // indirect github.com/mattn/go-localereader v0.0.1 // indirect github.com/microcosm-cc/bluemonday v1.0.27 // indirect @@ -41,7 +48,8 @@ require ( github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect github.com/yuin/goldmark v1.7.17 // indirect golang.org/x/net v0.38.0 // indirect - golang.org/x/sys v0.38.0 // indirect + golang.org/x/sync v0.19.0 // indirect + golang.org/x/sys v0.47.0 // indirect golang.org/x/term v0.36.0 // indirect golang.org/x/text v0.30.0 // indirect ) diff --git a/go.sum b/go.sum index d148b56..91b804c 100644 --- a/go.sum +++ b/go.sum @@ -16,28 +16,42 @@ github.com/charmbracelet/bubbles v1.0.0 h1:12J8/ak/uCZEMQ6KU7pcfwceyjLlWsDLAxB5f github.com/charmbracelet/bubbles v1.0.0/go.mod h1:9d/Zd5GdnauMI5ivUIVisuEm3ave1XwXtD1ckyV6r3E= github.com/charmbracelet/bubbletea v1.3.10 h1:otUDHWMMzQSB0Pkc87rm691KZ3SWa4KUlvF9nRvCICw= github.com/charmbracelet/bubbletea v1.3.10/go.mod h1:ORQfo0fk8U+po9VaNvnV95UPWA1BitP1E0N6xJPlHr4= -github.com/charmbracelet/colorprofile v0.4.1 h1:a1lO03qTrSIRaK8c3JRxJDZOvhvIeSco3ej+ngLk1kk= -github.com/charmbracelet/colorprofile v0.4.1/go.mod h1:U1d9Dljmdf9DLegaJ0nGZNJvoXAhayhmidOdcBwAvKk= +github.com/charmbracelet/colorprofile v0.4.2 h1:BdSNuMjRbotnxHSfxy+PCSa4xAmz7szw70ktAtWRYrY= +github.com/charmbracelet/colorprofile v0.4.2/go.mod h1:0rTi81QpwDElInthtrQ6Ni7cG0sDtwAd4C4le060fT8= github.com/charmbracelet/glamour v1.0.0 h1:AWMLOVFHTsysl4WV8T8QgkQ0s/ZNZo7CiE4WKhk8l08= github.com/charmbracelet/glamour v1.0.0/go.mod h1:DSdohgOBkMr2ZQNhw4LZxSGpx3SvpeujNoXrQyH2hxo= github.com/charmbracelet/lipgloss v1.1.1-0.20250404203927-76690c660834 h1:ZR7e0ro+SZZiIZD7msJyA+NjkCNNavuiPBLgerbOziE= github.com/charmbracelet/lipgloss v1.1.1-0.20250404203927-76690c660834/go.mod h1:aKC/t2arECF6rNOnaKaVU6y4t4ZeHQzqfxedE/VkVhA= -github.com/charmbracelet/x/ansi v0.11.6 h1:GhV21SiDz/45W9AnV2R61xZMRri5NlLnl6CVF7ihZW8= -github.com/charmbracelet/x/ansi v0.11.6/go.mod h1:2JNYLgQUsyqaiLovhU2Rv/pb8r6ydXKS3NIttu3VGZQ= +github.com/charmbracelet/ultraviolet v0.0.0-20260303162955-0b88c25f3fff h1:uY7A6hTokHPJBHfq7rj9Y/wm+IAjOghZTxKfVW6QLvw= +github.com/charmbracelet/ultraviolet v0.0.0-20260303162955-0b88c25f3fff/go.mod h1:E6/0abq9uG2SnM8IbLB9Y5SW09uIgfaFETk8aRzgXUQ= +github.com/charmbracelet/x/ansi v0.11.7 h1:kzv1kJvjg2S3r9KHo8hDdHFQLEqn4RBCb39dAYC84jI= +github.com/charmbracelet/x/ansi v0.11.7/go.mod h1:9qGpnAVYz+8ACONkZBUWPtL7lulP9No6p1epAihUZwQ= github.com/charmbracelet/x/cellbuf v0.0.15 h1:ur3pZy0o6z/R7EylET877CBxaiE1Sp1GMxoFPAIztPI= github.com/charmbracelet/x/cellbuf v0.0.15/go.mod h1:J1YVbR7MUuEGIFPCaaZ96KDl5NoS0DAWkskup+mOY+Q= +github.com/charmbracelet/x/conpty v0.2.0 h1:eKtA2hm34qNfgJCDp/M6Dc0gLy7e07YEK4qAdNGOvVY= +github.com/charmbracelet/x/conpty v0.2.0/go.mod h1:fexgUnVrZgw8scD49f6VSi0Ggj9GWYIrpedRthAwW/8= github.com/charmbracelet/x/exp/golden v0.0.0-20241011142426-46044092ad91 h1:payRxjMjKgx2PaCWLZ4p3ro9y97+TVLZNaRZgJwSVDQ= github.com/charmbracelet/x/exp/golden v0.0.0-20241011142426-46044092ad91/go.mod h1:wDlXFlCrmJ8J+swcL/MnGUuYnqgQdW9rhSD61oNMb6U= +github.com/charmbracelet/x/exp/ordered v0.1.0 h1:55/qLwjIh0gL0Vni+QAWk7T/qRVP6sBf+2agPBgnOFE= +github.com/charmbracelet/x/exp/ordered v0.1.0/go.mod h1:5UHwmG+is5THxMyCJHNPCn2/ecI07aKNrW+LcResjJ8= github.com/charmbracelet/x/exp/slice v0.0.0-20250327172914-2fdc97757edf h1:rLG0Yb6MQSDKdB52aGX55JT1oi0P0Kuaj7wi1bLUpnI= github.com/charmbracelet/x/exp/slice v0.0.0-20250327172914-2fdc97757edf/go.mod h1:B3UgsnsBZS/eX42BlaNiJkD1pPOUa+oF1IYC6Yd2CEU= github.com/charmbracelet/x/term v0.2.2 h1:xVRT/S2ZcKdhhOuSP4t5cLi5o+JxklsoEObBSgfgZRk= github.com/charmbracelet/x/term v0.2.2/go.mod h1:kF8CY5RddLWrsgVwpw4kAa6TESp6EB5y3uxGLeCqzAI= -github.com/clipperhouse/displaywidth v0.9.0 h1:Qb4KOhYwRiN3viMv1v/3cTBlz3AcAZX3+y9OLhMtAtA= -github.com/clipperhouse/displaywidth v0.9.0/go.mod h1:aCAAqTlh4GIVkhQnJpbL0T/WfcrJXHcj8C0yjYcjOZA= -github.com/clipperhouse/stringish v0.1.1 h1:+NSqMOr3GR6k1FdRhhnXrLfztGzuG+VuFDfatpWHKCs= -github.com/clipperhouse/stringish v0.1.1/go.mod h1:v/WhFtE1q0ovMta2+m+UbpZ+2/HEXNWYXQgCt4hdOzA= -github.com/clipperhouse/uax29/v2 v2.5.0 h1:x7T0T4eTHDONxFJsL94uKNKPHrclyFI0lm7+w94cO8U= -github.com/clipperhouse/uax29/v2 v2.5.0/go.mod h1:Wn1g7MK6OoeDT0vL+Q0SQLDz/KpfsVRgg6W7ihQeh4g= +github.com/charmbracelet/x/termios v0.1.1 h1:o3Q2bT8eqzGnGPOYheoYS8eEleT5ZVNYNy8JawjaNZY= +github.com/charmbracelet/x/termios v0.1.1/go.mod h1:rB7fnv1TgOPOyyKRJ9o+AsTU/vK5WHJ2ivHeut/Pcwo= +github.com/charmbracelet/x/vt v0.0.0-20260813141921-f091cedeaf78 h1:CIlgdpAf3PLGkv6Z+tISRNe6cnFoHVnjXtBWzo/NLAU= +github.com/charmbracelet/x/vt v0.0.0-20260813141921-f091cedeaf78/go.mod h1:u1LOIABor9JqY54oZdktK3TCRrgzP6tzHrDYx1nd3wY= +github.com/charmbracelet/x/windows v0.2.2 h1:IofanmuvaxnKHuV04sC0eBy/smG6kIKrWG2/jYn2GuM= +github.com/charmbracelet/x/windows v0.2.2/go.mod h1:/8XtdKZzedat74NQFn0NGlGL4soHB0YQZrETF96h75k= +github.com/charmbracelet/x/xpty v0.1.4 h1:4jaW7u+8AHQMxesiVc+zUMsspu7GyDwtJO+gy/tFtW4= +github.com/charmbracelet/x/xpty v0.1.4/go.mod h1:7t8P7BpPiolHJ1pLzz7/4ujDbD+sUxI9yA3CBOLOIcU= +github.com/clipperhouse/displaywidth v0.11.0 h1:lBc6kY44VFw+TDx4I8opi/EtL9m20WSEFgwIwO+UVM8= +github.com/clipperhouse/displaywidth v0.11.0/go.mod h1:bkrFNkf81G8HyVqmKGxsPufD3JhNl3dSqnGhOoSD/o0= +github.com/clipperhouse/uax29/v2 v2.7.0 h1:+gs4oBZ2gPfVrKPthwbMzWZDaAFPGYK72F0NJv2v7Vk= +github.com/clipperhouse/uax29/v2 v2.7.0/go.mod h1:EFJ2TJMRUaplDxHKj1qAEhCtQPW2tJSwu5BF98AuoVM= +github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= +github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= github.com/dlclark/regexp2 v1.11.5 h1:Q/sSnsKerHeCkc/jSTNq1oCm7KiVgUMZRDUoRu0JQZQ= github.com/dlclark/regexp2 v1.11.5/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8= github.com/erikgeiser/coninput v0.0.0-20211004153227-1c3628e74d0f h1:Y/CXytFA4m6baUTXGLOoWe4PQhGxaX0KpnayAqC48p4= @@ -46,15 +60,15 @@ github.com/gorilla/css v1.0.1 h1:ntNaBIghp6JmvWnxbZKANoLyuXTPZ4cAMlo6RyhlbO8= github.com/gorilla/css v1.0.1/go.mod h1:BvnYkspnSzMmwRK+b8/xgNPLiIuNZr6vbZBTPQ2A3b0= github.com/hexops/gotextdiff v1.0.3 h1:gitA9+qJrrTCsiCl7+kh75nPqQt1cx4ZkudSTLoUqJM= github.com/hexops/gotextdiff v1.0.3/go.mod h1:pSWU5MAI3yDq+fZBTazCSJysOMbxWL1BSow5/V2vxeg= -github.com/lucasb-eyer/go-colorful v1.3.0 h1:2/yBRLdWBZKrf7gB40FoiKfAWYQ0lqNcbuQwVHXptag= -github.com/lucasb-eyer/go-colorful v1.3.0/go.mod h1:R4dSotOR9KMtayYi1e77YzuveK+i7ruzyGqttikkLy0= +github.com/lucasb-eyer/go-colorful v1.4.0 h1:UtrWVfLdarDgc44HcS7pYloGHJUjHV/4FwW4TvVgFr4= +github.com/lucasb-eyer/go-colorful v1.4.0/go.mod h1:R4dSotOR9KMtayYi1e77YzuveK+i7ruzyGqttikkLy0= github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY= github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= github.com/mattn/go-localereader v0.0.1 h1:ygSAOl7ZXTx4RdPYinUpg6W99U8jWvWi9Ye2JC/oIi4= github.com/mattn/go-localereader v0.0.1/go.mod h1:8fBrzywKY7BI3czFoHkuzRoWE9C+EiG4R1k4Cjx5p88= github.com/mattn/go-runewidth v0.0.12/go.mod h1:RAqKPSqVFrSLVXbA8x7dzmKdmGzieGRCM46jaSJTDAk= -github.com/mattn/go-runewidth v0.0.19 h1:v++JhqYnZuu5jSKrk9RbgF5v4CGUjqRfBm05byFGLdw= -github.com/mattn/go-runewidth v0.0.19/go.mod h1:XBkDxAl56ILZc9knddidhrOlY5R/pDhgLpndooCuJAs= +github.com/mattn/go-runewidth v0.0.23 h1:7ykA0T0jkPpzSvMS5i9uoNn2Xy3R383f9HDx3RybWcw= +github.com/mattn/go-runewidth v0.0.23/go.mod h1:XBkDxAl56ILZc9knddidhrOlY5R/pDhgLpndooCuJAs= github.com/microcosm-cc/bluemonday v1.0.27 h1:MpEUotklkwCSLeH+Qdx1VJgNqLlpY2KXwXFM08ygZfk= github.com/microcosm-cc/bluemonday v1.0.27/go.mod h1:jFi9vgW+H7c3V0lb6nR74Ib/DIB5OBs92Dimizgw2cA= github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6 h1:ZK8zHtRHOkbHy6Mmr5D264iyp3TiX5OmNcI5cIARiQI= @@ -81,10 +95,12 @@ golang.org/x/mod v0.37.0 h1:vF1DjpVEshcIqoEaauuHebaLk1O1forxjxBaVn884JQ= golang.org/x/mod v0.37.0/go.mod h1:m8S8VeM9r4dzDwjrKO0a1sZP3YjeMamRRlD+fmR2Q/0= golang.org/x/net v0.38.0 h1:vRMAPTMaeGqVhG5QyLJHqNDwecKTomGeqbnfZyKlBI8= golang.org/x/net v0.38.0/go.mod h1:ivrbrMbzFq5J41QOQh0siUuly180yBYtLp+CKbEaFx8= +golang.org/x/sync v0.19.0 h1:vV+1eWNmZ5geRlYjzm2adRgW2/mcpevXNg50YZtPCE4= +golang.org/x/sync v0.19.0/go.mod h1:9KTHXmSnoGruLpwFjVSX0lNNA75CykiMECbovNTZqGI= golang.org/x/sys v0.0.0-20210809222454-d867a43fc93e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.38.0 h1:3yZWxaJjBmCWXqhN1qh02AkOnCQ1poK6oF+a7xWL6Gc= -golang.org/x/sys v0.38.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= +golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= +golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/term v0.36.0 h1:zMPR+aF8gfksFprF/Nc/rd1wRS1EI6nDBGyWAvDzx2Q= golang.org/x/term v0.36.0/go.mod h1:Qu394IJq6V6dCBRgwqshf3mPF85AqzYEzofzRdZkWss= golang.org/x/text v0.30.0 h1:yznKA/E9zq54KzlzBEAWn1NXSQ8DIp/NYMy88xJjl4k= diff --git a/internal/term/attr_unix.go b/internal/term/attr_unix.go new file mode 100644 index 0000000..f9d6af2 --- /dev/null +++ b/internal/term/attr_unix.go @@ -0,0 +1,19 @@ +//go:build !windows + +package term + +import "syscall" + +// ptyProcAttr puts the child in its own session with the pty slave as its +// controlling terminal. +// +// xpty does neither — UnixPty.Start only wires stdin/stdout/stderr to the slave +// — so without this the child inherits keepkit's controlling terminal, job +// control never engages, and Kill's process-group signal would land on +// keepkit's own group. +// +// Ctty is left at its zero value on purpose: it names a descriptor in the +// *child's* table, and fd 0 there is the slave xpty just attached as stdin. +func ptyProcAttr() *syscall.SysProcAttr { + return &syscall.SysProcAttr{Setsid: true, Setctty: true} +} diff --git a/internal/term/attr_windows.go b/internal/term/attr_windows.go new file mode 100644 index 0000000..81882c1 --- /dev/null +++ b/internal/term/attr_windows.go @@ -0,0 +1,11 @@ +//go:build windows + +package term + +import "syscall" + +// ptyProcAttr has nothing to add on Windows: ConPTY gives the child its own +// console, and the session-leader/controlling-terminal model the unix build +// needs Setsid/Setctty for does not exist here. ConPty.Start passes this +// straight through to the spawn as ProcAttr.Sys, where nil is fine. +func ptyProcAttr() *syscall.SysProcAttr { return nil } diff --git a/internal/term/session.go b/internal/term/session.go new file mode 100644 index 0000000..1c4a136 --- /dev/null +++ b/internal/term/session.go @@ -0,0 +1,171 @@ +// Package term runs a command on a pseudo-terminal and streams what it prints. +// +// It is the architectural slot the old tab launcher vacated: bottom of the +// import graph, no TUI knowledge. A Session owns the pty and one reader +// goroutine; everything it observes leaves through Events() as a value, so the +// model can consume it with the same waitForChunkCmd pattern the update +// streamer uses. The terminal *emulator* deliberately lives on the model, not +// here — only Bubble Tea's Update goroutine may touch its screen state. +// +// The caller builds argv (keepkit's model has shellCommand for that) and hands +// it to Start, which keeps this package goos-agnostic. +// +// This package writes no config and does no logx logging: a failure rides +// Exit.Err to the caller, which is the surface that can actually show it. That +// is why it needs no TestMain seam, unlike loader/version/logx. +package term + +import ( + "context" + "os" + "os/exec" + "sync" + "sync/atomic" + "time" + + "github.com/charmbracelet/x/xpty" + "github.com/stanlyzoolo/keepkit/internal/proc" +) + +// readChunk bounds one read off the pty master. A full-screen redraw of a +// large terminal is tens of kilobytes, so this sizes one repaint into one +// event rather than a dozen. +const readChunk = 32 * 1024 + +// Event is what a Session reports: Data for output, Exit exactly once, last. +type Event interface{ termEvent() } + +// Data is a chunk of bytes the child wrote. Bytes is owned by the receiver — +// the reader copies out of its buffer before sending. +type Data struct{ Bytes []byte } + +func (Data) termEvent() {} + +// Exit is the final event on the channel; the channel closes right after it. +// Err is nil for a clean exit and an *exec.ExitError for a non-zero one, so a +// caller can read the code off it. Elapsed is stamped in the reader goroutine, +// never by the consumer — a consumer that stamped it would make completion +// depend on when it got round to reading, which is not a testable quantity. +type Exit struct { + Err error + Elapsed time.Duration + // Killed reports that the exit followed a Kill() call rather than the + // child deciding to stop. The session is the only place that knows both + // halves of that, so it answers rather than making the caller correlate + // its own keypress against an exit status that says "signal: killed". + Killed bool +} + +func (Exit) termEvent() {} + +// Session is one command running on one pty. +type Session struct { + pty xpty.Pty + cmd *exec.Cmd + events chan Event + + killed atomic.Bool + closeOnce sync.Once +} + +// Start runs shell with args on a fresh w×h pty and begins streaming. +// +// env is the child's environment; nil inherits keepkit's. TERM and COLORTERM +// are appended either way — os/exec keeps the last value for a duplicate key, +// so appending overrides an inherited TERM without having to scan for it. The +// working directory is inherited. +// +// On unix the child is put in its own session with the pty slave as its +// controlling terminal. xpty does not do this itself (it only wires stdio), and +// without it job control never engages and the kill path would signal the wrong +// process group. It is also why proc.DetachTTY must never be applied here: it +// assigns SysProcAttr wholesale and would drop Setctty. +func Start(shell string, args []string, w, h int, env []string) (*Session, error) { + pty, err := xpty.NewPty(w, h) + if err != nil { + return nil, err + } + + if env == nil { + env = os.Environ() + } + cmd := exec.Command(shell, args...) + cmd.Env = append(append([]string{}, env...), "TERM=xterm-256color", "COLORTERM=truecolor") + cmd.SysProcAttr = ptyProcAttr() + + if err := pty.Start(cmd); err != nil { + _ = pty.Close() + return nil, err + } + + s := &Session{ + pty: pty, + cmd: cmd, + events: make(chan Event, eventBuffer), + } + go s.stream() + return s, nil +} + +// eventBuffer lets the reader stay ahead of a consumer that only drains once +// per Bubble Tea message, without letting output queue without bound: past it +// the reader blocks, the pty buffer fills, and the child is throttled — which +// is the correct back-pressure for a tool printing faster than we can paint. +const eventBuffer = 64 + +// Events returns the stream. It yields zero or more Data events, then exactly +// one Exit, then closes. +func (s *Session) Events() <-chan Event { return s.events } + +// stream is the one goroutine per session: pty master → Data events, then the +// process verdict → Exit, then close. +func (s *Session) stream() { + started := time.Now() + buf := make([]byte, readChunk) + + for { + n, err := s.pty.Read(buf) + if n > 0 { + chunk := make([]byte, n) + copy(chunk, buf[:n]) + s.events <- Data{Bytes: chunk} + } + if err != nil { + // Any read error ends the stream and none of them is the + // verdict. A master whose child has exited answers EIO on + // Linux and EOF on macOS, and a master closed by Kill/Close + // answers os.ErrClosed — all three are normal termination. + // What actually happened comes from the wait below. + break + } + } + + err := xpty.WaitProcess(context.Background(), s.cmd) + s.events <- Exit{Err: err, Elapsed: time.Since(started), Killed: s.killed.Load()} + close(s.events) +} + +// Write sends input to the child. +func (s *Session) Write(p []byte) (int, error) { return s.pty.Write(p) } + +// Resize tells the child its terminal changed size. +func (s *Session) Resize(w, h int) error { return s.pty.Resize(w, h) } + +// Kill terminates the child and everything it spawned. The unix child is a +// session leader (see Start), so its pid doubles as its process-group id and +// proc.KillGroup's negative-pid signal reaches the tools an `sh -c` line +// started. Best-effort: an already-exited process is a no-op, and the verdict +// still arrives as the Exit event once the reader drains. +func (s *Session) Kill() { + s.killed.Store(true) + _ = proc.KillGroup(s.cmd) +} + +// Close releases the pty. Calling it while the child lives sends it SIGHUP; +// the normal path is to Close after the Exit event has already arrived. Safe +// to call more than once. +func (s *Session) Close() error { + var err error + s.closeOnce.Do(func() { err = s.pty.Close() }) + return err +} diff --git a/internal/term/session_test.go b/internal/term/session_test.go new file mode 100644 index 0000000..01c9f4e --- /dev/null +++ b/internal/term/session_test.go @@ -0,0 +1,316 @@ +//go:build !windows + +package term + +import ( + "bytes" + "errors" + "io" + "os/exec" + "runtime" + "strings" + "testing" + "time" + + uv "github.com/charmbracelet/ultraviolet" + "github.com/charmbracelet/x/vt" +) + +// collect drains a session to its Exit event, returning everything the child +// printed plus the verdict. It fails rather than hangs if the session never +// finishes, so a regression shows up as one named test instead of a timeout on +// the whole package. +func collect(t *testing.T, s *Session, within time.Duration) ([]byte, Exit) { + t.Helper() + + var out []byte + deadline := time.After(within) + for { + select { + case ev, ok := <-s.Events(): + if !ok { + t.Fatal("channel closed before an Exit event") + } + switch e := ev.(type) { + case Data: + out = append(out, e.Bytes...) + case Exit: + // the channel must close right after the Exit + select { + case _, open := <-s.Events(): + if open { + t.Fatal("an event followed Exit") + } + case <-time.After(time.Second): + t.Fatal("channel did not close after Exit") + } + return out, e + } + case <-deadline: + t.Fatalf("session did not finish within %s", within) + } + } +} + +func startTest(t *testing.T, script string) *Session { + t.Helper() + + s, err := Start("sh", []string{"-c", script}, 80, 24, nil) + if err != nil { + t.Fatalf("Start: %v", err) + } + t.Cleanup(func() { _ = s.Close() }) + return s +} + +// A clean run streams its output and reports a nil error. This doubles as the +// EOF test: reading a pty master whose child has exited answers EIO on Linux +// and EOF on macOS, and neither may become the verdict. +func TestSessionRunsAndExitsClean(t *testing.T) { + s := startTest(t, "printf hi") + + out, exit := collect(t, s, 10*time.Second) + + if !bytes.Contains(out, []byte("hi")) { + t.Errorf("output %q does not contain %q", out, "hi") + } + if exit.Err != nil { + t.Errorf("Exit.Err = %v, want nil (a pty read error is not the verdict)", exit.Err) + } + if exit.Killed { + t.Error("Exit.Killed is true for a session nobody killed") + } + if exit.Elapsed <= 0 { + t.Errorf("Exit.Elapsed = %v, want > 0 (stamped in the reader, not by the consumer)", exit.Elapsed) + } +} + +// A non-zero exit rides Exit.Err as an *exec.ExitError, so the caller can read +// the code off it for the overlay's outcome line. +func TestSessionNonZeroExit(t *testing.T) { + s := startTest(t, "exit 3") + + _, exit := collect(t, s, 10*time.Second) + + var ee *exec.ExitError + if !errors.As(exit.Err, &ee) { + t.Fatalf("Exit.Err = %v (%T), want an *exec.ExitError", exit.Err, exit.Err) + } + if got := ee.ExitCode(); got != 3 { + t.Errorf("exit code %d, want 3", got) + } +} + +// A tool that is not installed is not a special case: sh reports it and exits +// 127, which is what the overlay shows. This replaces the old notFoundExit +// mapping the tab launcher needed. +func TestSessionNotInstalledToolExits127(t *testing.T) { + s := startTest(t, "keepkit-no-such-tool-xyz") + + out, exit := collect(t, s, 10*time.Second) + + var ee *exec.ExitError + if !errors.As(exit.Err, &ee) { + t.Fatalf("Exit.Err = %v, want an *exec.ExitError", exit.Err) + } + if got := ee.ExitCode(); got != 127 { + t.Errorf("exit code %d, want 127", got) + } + if !strings.Contains(strings.ToLower(string(out)), "not found") { + t.Errorf("output %q does not carry the shell's not-found message", out) + } +} + +// Kill terminates a child that would otherwise outlive the test, the channel +// closes, and the Exit says it was killed rather than leaving the caller to +// infer it from "signal: killed". +func TestSessionKill(t *testing.T) { + before := runtime.NumGoroutine() + s := startTest(t, "sleep 60") + + // let the child actually reach sleep before killing it + time.Sleep(150 * time.Millisecond) + s.Kill() + + _, exit := collect(t, s, 10*time.Second) + + if exit.Err == nil { + t.Error("Exit.Err is nil for a killed child") + } + if !exit.Killed { + t.Error("Exit.Killed is false after Kill()") + } + + // the reader goroutine must be gone once the channel closed + waitGoroutines(t, before) +} + +// Kill reaches the tools an `sh -c` line spawned, not just the shell: the child +// is a session leader, so the negative-pid signal covers the whole group. A +// grandchild that survived would keep the pty's write end open and the reader +// would never see EOF - which is exactly what this asserts by finishing. +func TestSessionKillReachesGrandchildren(t *testing.T) { + s := startTest(t, "sleep 60 & sleep 60") + + time.Sleep(150 * time.Millisecond) + s.Kill() + + _, exit := collect(t, s, 10*time.Second) + if !exit.Killed { + t.Error("Exit.Killed is false after Kill()") + } +} + +func TestSessionResize(t *testing.T) { + s := startTest(t, "sleep 1") + + if err := s.Resize(100, 30); err != nil { + t.Fatalf("Resize: %v", err) + } + + s.Kill() + collect(t, s, 10*time.Second) +} + +// Write reaches the child's stdin. +func TestSessionWrite(t *testing.T) { + s := startTest(t, "read line; printf 'got:%s' \"$line\"") + + time.Sleep(150 * time.Millisecond) + if _, err := s.Write([]byte("ping\r")); err != nil { + t.Fatalf("Write: %v", err) + } + + out, exit := collect(t, s, 10*time.Second) + if exit.Err != nil { + t.Errorf("Exit.Err = %v, want nil", exit.Err) + } + if !bytes.Contains(out, []byte("got:ping")) { + t.Errorf("output %q does not echo the written line", out) + } +} + +// A bogus command fails at Start and leaks nothing: no session is returned, so +// there is no channel to close and no goroutine to leak. +func TestStartBogusShell(t *testing.T) { + before := runtime.NumGoroutine() + + s, err := Start("keepkit-no-such-shell-xyz", nil, 80, 24, nil) + if err == nil { + _ = s.Close() + t.Fatal("Start with a bogus shell returned no error") + } + if s != nil { + t.Errorf("Start returned a session (%v) alongside an error", s) + } + + waitGoroutines(t, before) +} + +// Start's env is what the child sees, and TERM/COLORTERM are appended so the +// tool renders in colour. Appending is enough because os/exec keeps the last +// value for a duplicate key - this pins that, since an inherited TERM would +// otherwise win and a tmux/screen TERM would cost the overlay its colours. +func TestSessionEnvOverridesTerm(t *testing.T) { + s, err := Start("sh", []string{"-c", "printf '%s|%s' \"$TERM\" \"$COLORTERM\""}, + 80, 24, []string{"TERM=dumb"}) + if err != nil { + t.Fatalf("Start: %v", err) + } + t.Cleanup(func() { _ = s.Close() }) + + out, _ := collect(t, s, 10*time.Second) + + if !bytes.Contains(out, []byte("xterm-256color|truecolor")) { + t.Errorf("child saw %q, want TERM=xterm-256color and COLORTERM=truecolor", out) + } +} + +// The bytes a session streams are what a vt emulator turns back into a screen - +// this is the whole contract between internal/term and the model, checked once +// here so the model's own tests can use a fake session with a clear conscience. +func TestSessionOutputRendersInEmulator(t *testing.T) { + s := startTest(t, "printf 'hello pty'") + + out, _ := collect(t, s, 10*time.Second) + + emu := vt.NewEmulator(40, 5) + if _, err := emu.Write(out); err != nil { + t.Fatalf("emulator write: %v", err) + } + if !strings.Contains(emu.Render(), "hello pty") { + t.Errorf("emulator rendered %q, want it to contain %q", emu.Render(), "hello pty") + } +} + +// Closing the emulator's input pipe is how a copy goroutine parked in Read is +// stopped, and it must stay race-free: Emulator.Close() writes an +// unsynchronised bool that Read checks on every call (charmbracelet/x#879), so +// the obvious teardown is the one that trips -race. Closing InputPipe()'s +// writer goes through io.Pipe's own synchronisation instead and never touches +// that bool. The model's overlay teardown depends on this; see +// docs/research/pty-stack.md. +func TestSessionInputPipeCloseIsRaceFree(t *testing.T) { + emu := vt.NewEmulator(20, 5) + + var relayed []byte + done := make(chan struct{}) + go func() { + defer close(done) + buf := make([]byte, 64) + for { + n, err := emu.Read(buf) + if n > 0 { + relayed = append(relayed, buf[:n]...) + } + if err != nil { + return + } + } + }() + + // drive the screen state from this goroutine while the copy goroutine + // reads the input side: the two halves must not race either. + for range 50 { + if _, err := emu.Write([]byte("x")); err != nil { + t.Fatalf("emulator write: %v", err) + } + emu.SendKey(uv.KeyPressEvent{Code: 'a'}) + } + + closer, ok := emu.InputPipe().(io.Closer) + if !ok { + t.Fatalf("InputPipe() is %T, not an io.Closer - the overlay teardown assumes it is", emu.InputPipe()) + } + if err := closer.Close(); err != nil { + t.Fatalf("close input pipe: %v", err) + } + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("the copy goroutine did not exit after the input pipe was closed") + } + if len(relayed) != 50 { + t.Errorf("relayed %d bytes, want 50", len(relayed)) + } +} + +// waitGoroutines gives the runtime a moment to retire finished goroutines +// before comparing counts - an immediate read races the scheduler. +func waitGoroutines(t *testing.T, before int) { + t.Helper() + + deadline := time.Now().Add(5 * time.Second) + for { + got := runtime.NumGoroutine() + if got <= before { + return + } + if time.Now().After(deadline) { + t.Errorf("goroutines: %d before, %d after - the reader leaked", before, got) + return + } + time.Sleep(20 * time.Millisecond) + } +} From cc056dd9b2f3a93c560f216db633cad285c31368 Mon Sep 17 00:00:00 2001 From: stanlyzoolo <51911715+stanlyzoolo@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:31:09 +0300 Subject: [PATCH 03/13] feat: plumb the tool overlay into the model Adds modeToolOverlay and everything the embedded terminal needs on the model side, with the old launch path still intact: the mode is reachable by nothing yet, and its own tests are what keep unused quiet until the switch-over. The emulator lives on the Model rather than in internal/term so that only Update ever touches its screen state. Input needs a relay goroutine after all - x/vt exports no key encoder and encodes against modes it does not expose - so termInput has an explicit lifecycle with a single teardown that waits for it, and it stops by closing the emulator's input pipe rather than the emulator (upstream race x#879). The output drain folds queued chunks into one message and, when it runs into the exit, carries it along instead of swallowing it: a channel cannot be un-read, and delivering the exit before the data it followed would lose a short-lived tool's final screen, which is what esc-after- exit exists to show. Co-Authored-By: Claude Opus 5 (1M context) --- docs/plans/20260814-tool-overlay-terminal.md | 21 +- internal/model/mode.go | 10 +- internal/model/model.go | 40 ++ internal/model/mouse_test.go | 2 +- internal/model/overlay_term.go | 290 ++++++++++++++ internal/model/overlay_term_test.go | 383 +++++++++++++++++++ internal/model/render.go | 13 +- internal/model/zoom_test.go | 2 +- 8 files changed, 746 insertions(+), 15 deletions(-) create mode 100644 internal/model/overlay_term.go create mode 100644 internal/model/overlay_term_test.go diff --git a/docs/plans/20260814-tool-overlay-terminal.md b/docs/plans/20260814-tool-overlay-terminal.md index c824254..72a20b9 100644 --- a/docs/plans/20260814-tool-overlay-terminal.md +++ b/docs/plans/20260814-tool-overlay-terminal.md @@ -112,13 +112,20 @@ - Modify: `internal/model/mouse_test.go`, `internal/model/zoom_test.go` - Create: `internal/model/overlay_term_test.go` -- [ ] add `modeToolOverlay` to the `inputMode` enum; extend `overlayVisible()` with it — mouse gating rides along, **`View()`'s fg picker does not**: turn the `if m.mode == modeHotkeys` two-way pick into a `switch m.mode` with a placeholder `modeToolOverlay` body here, so Task 4 only fills the renderer in -- [ ] add model state: `termSession` behind a narrow local interface (the `restarter` idiom — `var _ termSession = (*term.Session)(nil)`), `termEmu`, `termExit`, `termW/termH`, `termToolName` -- [ ] add msgs `termStartedMsg`/`termChunkMsg`/`termExitMsg` and cmds `startTermCmd(shell, args, w, h)` (safeCmd-wrapped) + `waitForTermChunkCmd(session)` with the non-blocking drain that **stops at `Exit` and delivers accumulated data first**; handlers in `Update`: started → create emulator (Update-only rule) + wire the input path per Task 1's decision, chain the wait cmd; chunk → `termEmu.Write` + chain; exit → store `termExit`, stop chaining -- [ ] if Task 1 landed on the `InputPipe` copy goroutine: implement its explicit lifecycle — started exactly once per session when both ends exist; close order on cleanup is `Kill`/wait → close pty → close the emulator's input pipe → goroutine returns; it must never outlive `esc` -- [ ] add `modeToolOverlay` to the modal tables in `mouse_test.go` (mouse no-op set, ~line 201) and `zoom_test.go` (modal `z` guard, ~line 287) — these tables are what "rides along" means -- [ ] write tests: handlers drive a fake session (chunk channel + recorded `Write`/`Kill`/`Resize`); chunk msg reaches a real `vt` emulator and `Render()` shows the bytes (pure Go — no pty); a fake whose channel holds data+data+Exit yields both data chunks before `termExitMsg` and `Render()` shows all of it; exit msg stores status and stops the chain; if the pipe goroutine exists — a `-race` leak test for its close order -- [ ] run the full gate (build + vet + `go test -race ./...` + lint) - must pass before task 3 +- [x] add `modeToolOverlay` to the `inputMode` enum; extend `overlayVisible()` with it — mouse gating rides along, **`View()`'s fg picker does not**: turn the `if m.mode == modeHotkeys` two-way pick into a `switch m.mode` with a placeholder `modeToolOverlay` body here, so Task 4 only fills the renderer in +- [x] add model state: `termSession` behind a narrow local interface (the `restarter` idiom — `var _ termSession = (*term.Session)(nil)`), `termEmu`, `termExit`, `termW/termH`, `termToolName` + - ➕ plus `termInput *termInput`, the relay's handle (Task 1's drift 1 made the goroutine unavoidable). `termExit != nil` is the single "has it finished" discriminator — no separate flag, no "running" enum member, mirroring `selfState`'s derived `selfUpdating()`. +- [x] add msgs `termStartedMsg`/`termChunkMsg`/`termExitMsg` and cmds `startTermCmd(shell, args, w, h)` (safeCmd-wrapped) + `waitForTermChunkCmd(session)` with the non-blocking drain that **stops at `Exit` and delivers accumulated data first**; handlers in `Update`: started → create emulator (Update-only rule) + wire the input path per Task 1's decision, chain the wait cmd; chunk → `termEmu.Write` + chain; exit → store `termExit`, stop chaining + - ➕ **the drain's exit rides `termChunkMsg.exit *termExitMsg`**: a Go channel cannot be un-read, so the exit the drain already consumed had nowhere else to go. The handler writes the data, then re-emits the exit as the *next* message — the plan's stated ordering, literally. + - ➕ `termExitMsg` carries **`startFailed`** so the outcome line can say `✕ failed to start` without reconstructing it from a nil session. + - ➕ **stale-session gating**: a chunk whose session is not `m.termSession` is dropped **without re-subscribing** (a dead session's channel must not keep a command chain alive), and a `termStartedMsg` arriving outside the mode is **killed and closed** rather than adopted — otherwise a start that lost its race with `esc` strands a live pty. +- [x] if Task 1 landed on the `InputPipe` copy goroutine: implement its explicit lifecycle — started exactly once per session when both ends exist; close order on cleanup is `Kill`/wait → close pty → close the emulator's input pipe → goroutine returns; it must never outlive `esc` + - implemented as `termInput.stop(emu)`, called by the single teardown `closeToolOverlay()` in exactly that order, and it **waits** on the goroutine's done channel. +- [x] add `modeToolOverlay` to the modal tables in `mouse_test.go` (mouse no-op set, ~line 201) and `zoom_test.go` (modal `z` guard, ~line 287) — these tables are what "rides along" means + - ⚠️ **plan ordering flaw**: the `zoom_test.go` row cannot be green until the mode-dispatch case exists, which the plan put in Task 3. Pulled the `case modeToolOverlay:` dispatch (and a `updateToolOverlay` that consumes every key) forward into Task 2 — a mode that owns all input is the truthful placeholder, and it is what the table asserts. Task 3 fills in translation, the kill chord and esc. +- [x] write tests: handlers drive a fake session (chunk channel + recorded `Write`/`Kill`/`Resize`); chunk msg reaches a real `vt` emulator and `Render()` shows the bytes (pure Go — no pty); a fake whose channel holds data+data+Exit yields both data chunks before `termExitMsg` and `Render()` shows all of it; exit msg stores status and stops the chain; if the pipe goroutine exists — a `-race` leak test for its close order +- [x] run the full gate (build + vet + `go test -race ./...` + lint) - must pass before task 3 + - ⚠️ **golangci-lint's cache lies across a package's file set**: adding `overlay_term.go` made it report 6 `SA5011` false positives in three *untouched* test files. `golangci-lint cache clean` → 0 issues, before and after. Not a code problem; worth knowing before anyone "fixes" a `t.Fatalf` helper that was never broken. ### Task 3: input routing — `updateToolOverlay` handler and key translation diff --git a/internal/model/mode.go b/internal/model/mode.go index 945ee68..ff8aa49 100644 --- a/internal/model/mode.go +++ b/internal/model/mode.go @@ -33,6 +33,7 @@ const ( modeAPIStatus // "a": rate-limit / token overlay modeTokenInput // "e" inside the overlay: masked token entry modeHotkeys // "?": static hotkeys-help overlay + modeToolOverlay // enter in focusTools: the tool runs in an embedded terminal ) // apiOverlayVisible reports whether the API-status overlay is on screen — @@ -42,11 +43,12 @@ func (m Model) apiOverlayVisible() bool { } // overlayVisible reports whether any modal overlay is composited over the -// layout — the [a] API-status overlay (incl. token entry) or the [?] hotkeys -// overlay. It is the single "modal on screen" predicate for View() and the -// mouse gate, so a new overlay only has to extend this one helper. +// layout — the [a] API-status overlay (incl. token entry), the [?] hotkeys +// overlay, or the embedded tool terminal. It is the single "modal on screen" +// predicate for View() and the mouse gate, so a new overlay only has to extend +// this one helper. func (m Model) overlayVisible() bool { - return m.apiOverlayVisible() || m.mode == modeHotkeys + return m.apiOverlayVisible() || m.mode == modeHotkeys || m.mode == modeToolOverlay } // updateHotkeys handles keys while the [?] hotkeys overlay is open: esc, q, or diff --git a/internal/model/model.go b/internal/model/model.go index 6017e1f..dcddfd7 100644 --- a/internal/model/model.go +++ b/internal/model/model.go @@ -13,6 +13,7 @@ import ( "github.com/charmbracelet/bubbles/viewport" tea "github.com/charmbracelet/bubbletea" "github.com/charmbracelet/lipgloss" + "github.com/charmbracelet/x/vt" "github.com/stanlyzoolo/keepkit/internal/loader" "github.com/stanlyzoolo/keepkit/internal/logx" @@ -335,6 +336,29 @@ type Model struct { pendingLaunchName string pendingLaunchCommand string + // The embedded tool terminal (modeToolOverlay). termSession is the running + // tool behind a narrow interface so tests can drive the handlers with a + // fake; termEmu is the VT emulator turning its bytes back into a screen and + // lives here, on the model, precisely so that only Update ever touches its + // screen state. termInput is the goroutine relaying encoded keys back to + // the tool (see its type doc for why one is needed at all). + // + // termExit is nil while the tool runs and holds the verdict afterwards — it + // is the single "has it finished" discriminator, which is why there is no + // separate flag and no "running" member on any enum. termW/termH are the + // emulator's body size, resolved from termGeometry at dispatch, and + // termToolName is what the frame and the status bar call it. + // + // The whole group is nil/zero outside the overlay: closeToolOverlay is the + // single teardown and clears every field, so a stale session can never + // outlive the mode. + termSession termSession + termEmu *vt.Emulator + termInput *termInput + termExit *termExitMsg + termW, termH int + termToolName string + // spinner animates while a force refresh ([r]) is in flight; refreshingFor // holds the name of the tool being refreshed (empty = idle). refreshingFor // doubles as the double-press guard and as the tick-loop / render gate. @@ -1411,6 +1435,16 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.briefViewport.SetContent(m.renderCard()) return m, tea.Batch(statusCmd, fetchInstalledCmd(t)) + case termStartedMsg: + return m, m.handleTermStarted(msg) + + case termChunkMsg: + return m, m.handleTermChunk(msg) + + case termExitMsg: + m.handleTermExit(msg) + return m, nil + case statusExpiredMsg: // Retire a transient status only if it is still the current one: a stale // timer from a superseded message must not clear the newer message. The @@ -1453,6 +1487,12 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return flushPendingLaunch(m.updateAPIStatus(msg)) case modeHotkeys: return flushPendingLaunch(m.updateHotkeys(msg)) + case modeToolOverlay: + // Deliberately not wrapped in flushPendingLaunch, unlike every + // sibling above: a deferred exec fallback must not seize the + // terminal with tea.ExecProcess on the very keystroke that closes + // the overlay. The wrapper disappears with the tab launcher. + return m.updateToolOverlay(msg) } if m.mode == modeSearch { diff --git a/internal/model/mouse_test.go b/internal/model/mouse_test.go index 9ea4f02..ce4891b 100644 --- a/internal/model/mouse_test.go +++ b/internal/model/mouse_test.go @@ -198,7 +198,7 @@ func TestMouseNoOpUnderOverlay(t *testing.T) { for i := range names { names[i] = fmt.Sprintf("tool%02d", i) } - for _, mode := range []inputMode{modeAPIStatus, modeTokenInput, modeHotkeys} { + for _, mode := range []inputMode{modeAPIStatus, modeTokenInput, modeHotkeys, modeToolOverlay} { t.Run(fmt.Sprintf("mode_%d", mode), func(t *testing.T) { m := newMouseTestModel(t, 80, 10, names...) m.mode = mode diff --git a/internal/model/overlay_term.go b/internal/model/overlay_term.go new file mode 100644 index 0000000..f0e7673 --- /dev/null +++ b/internal/model/overlay_term.go @@ -0,0 +1,290 @@ +package model + +import ( + "io" + "time" + + tea "github.com/charmbracelet/bubbletea" + "github.com/charmbracelet/x/vt" + + "github.com/stanlyzoolo/keepkit/internal/term" +) + +// termSession is the model's view of a running tool: everything the overlay +// needs and nothing else, so a test can drive the handlers with a fake instead +// of spawning a real pty. The narrow-interface idiom main.go uses for +// restarter, and the package-level assertion below is what keeps the real +// session bound to it. +type termSession interface { + Events() <-chan term.Event + Write(p []byte) (int, error) + Resize(w, h int) error + Kill() + Close() error +} + +var _ termSession = (*term.Session)(nil) + +// termStartedMsg carries the live session back to Update, which is where the +// emulator is created — the emulator's screen state is touched by Update and +// nothing else, so it cannot be built in the command goroutine. +type termStartedMsg struct { + session termSession +} + +// termChunkMsg is one drained batch of the tool's output. +// +// exit is set when the drain ran into the session's final event. It rides along +// rather than being dropped because the drain has already taken it off the +// channel and there is nowhere to put it back: the handler writes data to the +// emulator first and re-emits exit as the *next* message, which is what keeps a +// short-lived CLI's final screen on display. Losing it the other way round — +// exit first, data discarded — would defeat the whole esc-after-exit design. +type termChunkMsg struct { + session termSession + data []byte + exit *termExitMsg +} + +// termExitMsg is the tool's verdict. elapsed and killed are stamped by the +// session goroutine, never here: time.Now() inside Update would make completion +// non-deterministic in tests, and only the session knows whether the exit +// followed a kill. +// +// startFailed marks the case where there is no session at all — term.Start +// itself failed, so the overlay opens straight into its outcome line. It is an +// explicit field rather than an inference from a nil m.termSession because the +// renderer reads it long after the distinction would have to be reconstructed. +type termExitMsg struct { + err error + elapsed time.Duration + killed bool + startFailed bool +} + +// exitMsgFrom converts the session's final event into the model's message. +func exitMsgFrom(e term.Exit) termExitMsg { + return termExitMsg{err: e.Err, elapsed: e.Elapsed, killed: e.Killed} +} + +// termInput relays the emulator's encoded input to the tool. +// +// This goroutine exists because the pinned x/vt has no exported key encoder and +// its SendKey encodes against emulator modes (DECCKM — whether arrows are +// \x1bOA or \x1b[A) that no accessor exposes, so keys must go through the +// emulator and come back out of its input pipe. Update calls SendKey/SendText; +// this goroutine is the reader that unblocks the internal io.Pipe those write +// into, and it touches no screen state — the Update-only rule holds. +type termInput struct { + done chan struct{} +} + +// startTermInput wires emu's input side to sess. One goroutine per session, +// started exactly once, when both ends exist. +func startTermInput(emu *vt.Emulator, sess termSession) *termInput { + ti := &termInput{done: make(chan struct{})} + go func() { + defer close(ti.done) + buf := make([]byte, 256) + for { + n, err := emu.Read(buf) + if n > 0 { + if _, werr := sess.Write(buf[:n]); werr != nil { + return + } + } + if err != nil { + return + } + } + }() + return ti +} + +// stop ends the relay and waits for the goroutine, so it can never outlive the +// overlay. Callers close the session first: that way a relay parked in +// Session.Write fails fast instead of holding the teardown. +// +// It closes the emulator's *input pipe* rather than the emulator. Emulator.Close +// writes an unsynchronised bool that Emulator.Read checks on every call +// (charmbracelet/x#879) — reproduced under -race in internal/term — while +// closing the pipe writer unblocks the parked Read through io.Pipe's own +// synchronisation and touches nothing shared. The Close fallback is for a future +// x/vt that stops handing out an io.Closer; see docs/research/pty-stack.md. +func (ti *termInput) stop(emu *vt.Emulator) { + if ti == nil { + return + } + if emu != nil { + if closer, ok := emu.InputPipe().(io.Closer); ok { + _ = closer.Close() + } else { + _ = emu.Close() + } + } + <-ti.done +} + +// startTermCmd runs shell+args on a fresh w×h pty off the Update thread. +// +// A start failure never reaches the caller as an error: it becomes an immediate +// termExitMsg, so the overlay that just opened shows why instead of closing +// again under the user. +func startTermCmd(shell string, args []string, w, h int) tea.Cmd { + return safeCmd("startTermCmd", func() tea.Msg { + s, err := term.Start(shell, args, w, h, nil) + if err != nil { + return termExitMsg{err: err, startFailed: true} + } + return termStartedMsg{session: s} + }) +} + +// waitForTermChunkCmd blocks for the session's next event and folds everything +// already queued behind it into one message — a full-screen redraw arrives as +// several reads and repainting once per read would spend a Bubble Tea frame on +// each. It is the re-subscribe half of the channel idiom waitForChunkCmd uses +// for the update streamer. +// +// The drain stops at the final event and hands it back through termChunkMsg.exit +// rather than swallowing it; see that field's doc for why the order matters. +func waitForTermChunkCmd(s termSession) tea.Cmd { + return safeCmd("waitForTermChunkCmd", func() tea.Msg { + ev, ok := <-s.Events() + if !ok { + // A closed channel with no Exit behind it is not something the + // session can produce, but a fake can: end the overlay rather + // than re-subscribing to a channel that will never answer. + return termExitMsg{} + } + + switch e := ev.(type) { + case term.Exit: + return exitMsgFrom(e) + case term.Data: + data := e.Bytes + for { + select { + case next, open := <-s.Events(): + if !open { + return termChunkMsg{session: s, data: data, exit: &termExitMsg{}} + } + switch n := next.(type) { + case term.Data: + data = append(data, n.Bytes...) + case term.Exit: + exit := exitMsgFrom(n) + return termChunkMsg{session: s, data: data, exit: &exit} + } + default: + return termChunkMsg{session: s, data: data} + } + } + } + return nil + }) +} + +// handleTermStarted adopts the live session: the emulator is created here, on +// the Update goroutine, and the input relay is started now that both ends +// exist. +// +// A session arriving after the overlay is gone — the user's tool failed to +// start and esc closed the outcome line while term.Start was still in flight, +// or a stale start lost its race — is killed rather than adopted. Leaving it +// running would strand a pty nothing can reach. +func (m *Model) handleTermStarted(msg termStartedMsg) tea.Cmd { + if m.mode != modeToolOverlay || m.termSession != nil { + msg.session.Kill() + _ = msg.session.Close() + return nil + } + + m.termSession = msg.session + m.termEmu = vt.NewEmulator(m.termW, m.termH) + m.termInput = startTermInput(m.termEmu, msg.session) + return waitForTermChunkCmd(msg.session) +} + +// handleTermChunk folds one batch of output into the emulator. +// +// A chunk whose session is not the current one is stale — its overlay is +// already closed — and is dropped without re-subscribing, so a dead session's +// channel cannot keep a command chain alive. +func (m *Model) handleTermChunk(msg termChunkMsg) tea.Cmd { + if msg.session == nil || msg.session != m.termSession { + return nil + } + + if m.termEmu != nil && len(msg.data) > 0 { + _, _ = m.termEmu.Write(msg.data) + } + + if msg.exit != nil { + // Deliver the exit as the next message, after this batch has been + // painted: the final screen is the point of staying open. + exit := *msg.exit + return func() tea.Msg { return exit } + } + return waitForTermChunkCmd(msg.session) +} + +// handleTermExit records the verdict and stops the chain. It writes nothing +// else: the overlay stays exactly as the tool left it until esc, and the +// screen is the answer — no status message. +func (m *Model) handleTermExit(msg termExitMsg) { + if m.mode != modeToolOverlay { + return + } + exit := msg + m.termExit = &exit +} + +// closeToolOverlay tears the overlay down and returns to modeNormal. +// +// The order is load-bearing: the session goes first so the relay's pending +// Session.Write fails fast, then the relay is stopped and waited for, and only +// then are the emulator and the rest of the state dropped. Reversing the first +// two can park the relay in a write nobody will drain. +func (m *Model) closeToolOverlay() { + if m.termSession != nil { + m.termSession.Kill() + _ = m.termSession.Close() + } + m.termInput.stop(m.termEmu) + + m.termSession = nil + m.termInput = nil + m.termEmu = nil + m.termExit = nil + m.termToolName = "" + m.termW, m.termH = 0, 0 + m.mode = modeNormal +} + +// updateToolOverlay owns every keystroke while the overlay is open — that is +// what "the tool has the keyboard" means, and it is why the mode consumes keys +// it has no use for rather than letting them fall through to the normal-mode +// map underneath. Key translation, the ctrl+\ kill chord and esc-after-exit +// land in Task 3 of the tool-overlay plan. +func (m Model) updateToolOverlay(_ tea.KeyMsg) (tea.Model, tea.Cmd) { + return m, nil +} + +// renderToolOverlay draws the embedded terminal. Placeholder: the frame, +// geometry, cursor and outcome line land in Task 4 of the tool-overlay plan. +func (m Model) renderToolOverlay() string { + body := "starting " + m.termToolName + "…" + if m.termEmu != nil { + body = m.termEmu.Render() + } + return m.sty().OverlayBorder.Render(body) +} + +// termRunning reports that the overlay has a tool that has not exited yet — +// the state where every key belongs to the tool and only ctrl+\ is reserved. +// A session still starting counts as running: its keys are dropped, not +// treated as an exited overlay's. +func (m Model) termRunning() bool { + return m.mode == modeToolOverlay && m.termExit == nil +} diff --git a/internal/model/overlay_term_test.go b/internal/model/overlay_term_test.go new file mode 100644 index 0000000..0e469e6 --- /dev/null +++ b/internal/model/overlay_term_test.go @@ -0,0 +1,383 @@ +package model + +import ( + "errors" + "strings" + "sync" + "testing" + "time" + + tea "github.com/charmbracelet/bubbletea" + + "github.com/stanlyzoolo/keepkit/internal/term" +) + +// fakeTermSession drives the overlay handlers without a pty: the test feeds +// events in and reads back what the model wrote, killed or resized. Every +// method is safe to call from the input relay goroutine, so the fake is usable +// under -race. +type fakeTermSession struct { + events chan term.Event + + mu sync.Mutex + written []byte + killed int + closed int + resized [][2]int + writeIn error +} + +func newFakeSession(buffer int) *fakeTermSession { + return &fakeTermSession{events: make(chan term.Event, buffer)} +} + +func (f *fakeTermSession) Events() <-chan term.Event { return f.events } + +func (f *fakeTermSession) Write(p []byte) (int, error) { + f.mu.Lock() + defer f.mu.Unlock() + if f.writeIn != nil { + return 0, f.writeIn + } + f.written = append(f.written, p...) + return len(p), nil +} + +func (f *fakeTermSession) Resize(w, h int) error { + f.mu.Lock() + defer f.mu.Unlock() + f.resized = append(f.resized, [2]int{w, h}) + return nil +} + +func (f *fakeTermSession) Kill() { + f.mu.Lock() + defer f.mu.Unlock() + f.killed++ +} + +func (f *fakeTermSession) Close() error { + f.mu.Lock() + defer f.mu.Unlock() + f.closed++ + return nil +} + +// input returns everything the model has relayed to the tool so far. It polls, +// because the relay is a goroutine and a bare read would race the handoff. +func (f *fakeTermSession) input(t *testing.T, want string) string { + t.Helper() + + deadline := time.Now().Add(2 * time.Second) + for { + f.mu.Lock() + got := string(f.written) + f.mu.Unlock() + if want == "" || strings.Contains(got, want) || time.Now().After(deadline) { + return got + } + time.Sleep(5 * time.Millisecond) + } +} + +func (f *fakeTermSession) counts() (killed, closed int) { + f.mu.Lock() + defer f.mu.Unlock() + return f.killed, f.closed +} + +// overlayModel is a model sitting in modeToolOverlay with the session already +// adopted, i.e. the state everything after termStartedMsg runs in. +func overlayModel(t *testing.T, f *fakeTermSession) Model { + t.Helper() + + m := newTestModel(focusTools) + m.mode = modeToolOverlay + m.termToolName = "git" + m.termW, m.termH = 40, 10 + + nm := mustModel(m.Update(termStartedMsg{session: f})) + if nm.termSession == nil { + t.Fatal("termStartedMsg did not adopt the session") + } + if nm.termEmu == nil { + t.Fatal("termStartedMsg did not create the emulator") + } + t.Cleanup(func() { nm.closeToolOverlay() }) + return nm +} + +// The started message is what creates the emulator - on the Update goroutine, +// which is the whole reason the emulator lives on the model and not inside +// internal/term. +func TestTermStartedCreatesEmulatorAtGeometrySize(t *testing.T) { + f := newFakeSession(4) + m := overlayModel(t, f) + + if got := m.termEmu.Width(); got != 40 { + t.Errorf("emulator width = %d, want 40 (termW)", got) + } + if got := m.termEmu.Height(); got != 10 { + t.Errorf("emulator height = %d, want 10 (termH)", got) + } + if m.termExit != nil { + t.Error("termExit is set on a session that just started") + } + if !m.termRunning() { + t.Error("termRunning() is false right after the session started") + } +} + +// A session that arrives after the overlay is gone must not be adopted and must +// not be left running: nothing could reach it afterwards. +func TestTermStartedAfterCloseKillsTheSession(t *testing.T) { + f := newFakeSession(4) + m := newTestModel(focusTools) // still modeNormal - the overlay never opened + + nm := mustModel(m.Update(termStartedMsg{session: f})) + + if nm.termSession != nil { + t.Error("a session was adopted outside modeToolOverlay") + } + killed, closed := f.counts() + if killed == 0 || closed == 0 { + t.Errorf("stale session left running: killed=%d closed=%d, want both > 0", killed, closed) + } +} + +// A chunk reaches a real vt emulator and shows up in Render() - pure Go, no +// pty involved, which is what makes the whole overlay assertable in model +// tests. +func TestTermChunkReachesTheEmulator(t *testing.T) { + f := newFakeSession(4) + m := overlayModel(t, f) + + nm := mustModel(m.Update(termChunkMsg{session: f, data: []byte("hello overlay")})) + + if got := nm.termEmu.Render(); !strings.Contains(got, "hello overlay") { + t.Errorf("emulator rendered %q, want it to contain %q", got, "hello overlay") + } +} + +// A chunk belonging to a session the model no longer holds is dropped, and - +// the part that matters - it does not re-subscribe: a dead session's channel +// must not keep a command chain alive for the rest of the process. +func TestTermChunkFromStaleSessionIsDropped(t *testing.T) { + f := newFakeSession(4) + m := overlayModel(t, f) + + stale := newFakeSession(4) + nm, cmd := m.Update(termChunkMsg{session: stale, data: []byte("ghost")}) + + if cmd != nil { + t.Error("a stale chunk re-subscribed to its session") + } + if got := nm.(Model).termEmu.Render(); strings.Contains(got, "ghost") { + t.Error("a stale chunk was written to the live emulator") + } +} + +// The drain folds queued output into one message and, when it runs into the +// exit, delivers the data first and the exit as the next message. Losing that +// order loses a short-lived CLI's final screen, which is the whole point of +// staying open until esc. +func TestWaitForTermChunkDeliversDataBeforeExit(t *testing.T) { + f := newFakeSession(8) + f.events <- term.Data{Bytes: []byte("first ")} + f.events <- term.Data{Bytes: []byte("second")} + f.events <- term.Exit{Err: nil, Elapsed: 3 * time.Second} + close(f.events) + + msg := waitForTermChunkCmd(f)() + + chunk, ok := msg.(termChunkMsg) + if !ok { + t.Fatalf("first message is %T, want termChunkMsg", msg) + } + if got := string(chunk.data); got != "first second" { + t.Errorf("drained data = %q, want %q - the drain did not fold both chunks", got, "first second") + } + if chunk.exit == nil { + t.Fatal("the drain reached the exit and dropped it") + } + if chunk.exit.elapsed != 3*time.Second { + t.Errorf("elapsed = %v, want 3s (stamped by the session, not by Update)", chunk.exit.elapsed) + } + + // and the model plays them out in that order: emulator first, exit next + m := overlayModel(t, f) + nm, cmd := m.Update(chunk) + if got := nm.(Model).termEmu.Render(); !strings.Contains(got, "first second") { + t.Errorf("emulator rendered %q, want the drained data", got) + } + if nm.(Model).termExit != nil { + t.Error("the exit was recorded in the same update as the data it must follow") + } + if cmd == nil { + t.Fatal("no follow-up command carried the exit") + } + if _, ok := cmd().(termExitMsg); !ok { + t.Errorf("follow-up message is %T, want termExitMsg", cmd()) + } +} + +// A drain that starts on the exit reports it directly. +func TestWaitForTermChunkExitOnly(t *testing.T) { + f := newFakeSession(2) + f.events <- term.Exit{Err: errors.New("boom"), Elapsed: time.Second, Killed: true} + + msg := waitForTermChunkCmd(f)() + + exit, ok := msg.(termExitMsg) + if !ok { + t.Fatalf("message is %T, want termExitMsg", msg) + } + if exit.err == nil || !exit.killed { + t.Errorf("exit = %+v, want the error and killed flag carried through", exit) + } +} + +// The exit message stores the verdict and stops the chain. It writes no status +// message: the screen is the answer. +func TestTermExitStoresVerdictAndStopsTheChain(t *testing.T) { + f := newFakeSession(4) + m := overlayModel(t, f) + + nm, cmd := m.Update(termExitMsg{err: errors.New("exit status 1"), elapsed: 2 * time.Second}) + got := nm.(Model) + + if got.termExit == nil { + t.Fatal("termExit is nil after termExitMsg") + } + if got.termExit.elapsed != 2*time.Second { + t.Errorf("elapsed = %v, want 2s", got.termExit.elapsed) + } + if cmd != nil { + t.Error("termExitMsg returned a command - the chain must stop at the exit") + } + if got.termRunning() { + t.Error("termRunning() is still true after the exit") + } + if got.statusMsg != "" { + t.Errorf("statusMsg = %q, want empty - the overlay itself reports the outcome", got.statusMsg) + } + if got.mode != modeToolOverlay { + t.Errorf("mode = %v, want modeToolOverlay - the overlay stays until esc", got.mode) + } +} + +// An exit arriving after the overlay closed changes nothing. +func TestTermExitOutsideOverlayIsIgnored(t *testing.T) { + m := newTestModel(focusTools) + + nm := mustModel(m.Update(termExitMsg{err: errors.New("late")})) + + if nm.termExit != nil { + t.Error("a late exit was recorded outside the overlay") + } +} + +// A start failure never reaches the user as a closed overlay: it arrives as an +// exit that the outcome line can explain. +func TestStartTermCmdFailureBecomesExit(t *testing.T) { + msg := startTermCmd("keepkit-no-such-shell-xyz", nil, 40, 10)() + + exit, ok := msg.(termExitMsg) + if !ok { + t.Fatalf("message is %T, want termExitMsg", msg) + } + if exit.err == nil { + t.Error("a failed start carried no error") + } + if !exit.startFailed { + t.Error("startFailed is false on a start that failed") + } +} + +// The input relay carries what Update sends to the tool, and the emulator is +// what encodes it - which is why the relay exists at all (x/vt has no exported +// key encoder). This is the -race lifecycle test: the relay must relay while +// the screen is being written from Update, and must be gone after teardown. +func TestTermInputRelayAndTeardown(t *testing.T) { + f := newFakeSession(4) + m := overlayModel(t, f) + + m.termEmu.SendText("ls -la\r") + // drive the screen from this goroutine at the same time: the relay must + // not touch screen state + if _, err := m.termEmu.Write([]byte("banner")); err != nil { + t.Fatalf("emulator write: %v", err) + } + + if got := f.input(t, "ls -la"); !strings.Contains(got, "ls -la\r") { + t.Errorf("tool received %q, want it to contain %q", got, "ls -la\r") + } + + done := m.termInput.done + m.closeToolOverlay() + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("the input relay outlived the overlay") + } + if m.termSession != nil || m.termEmu != nil || m.termInput != nil || m.termExit != nil { + t.Error("closeToolOverlay left overlay state behind") + } + if m.mode != modeNormal { + t.Errorf("mode = %v after teardown, want modeNormal", m.mode) + } + killed, closed := f.counts() + if killed == 0 || closed == 0 { + t.Errorf("teardown left the session alive: killed=%d closed=%d", killed, closed) + } +} + +// Teardown is safe on an overlay that never got a session - the window between +// the keypress and termStartedMsg. +func TestCloseToolOverlayBeforeStart(t *testing.T) { + m := newTestModel(focusTools) + m.mode = modeToolOverlay + m.termToolName = "git" + + m.closeToolOverlay() // must not panic on the nil session/emulator/relay + + if m.mode != modeNormal { + t.Errorf("mode = %v, want modeNormal", m.mode) + } +} + +// The third overlay rides the shared modal predicate, which is what gates the +// mouse and View(). +func TestToolOverlayIsAnOverlay(t *testing.T) { + m := newTestModel(focusTools) + m.mode = modeToolOverlay + + if !m.overlayVisible() { + t.Error("overlayVisible() = false in modeToolOverlay") + } + if m.apiOverlayVisible() { + t.Error("apiOverlayVisible() = true in modeToolOverlay - it is a different overlay") + } +} + +// View picks the tool overlay rather than falling through to the API-status +// panel, which is what the old two-way if would have done. +func TestViewPicksTheToolOverlay(t *testing.T) { + f := newFakeSession(4) + m := overlayModel(t, f) + m.ready = true + m.applyLayout() + + nm := mustModel(m.Update(termChunkMsg{session: f, data: []byte("in-overlay-marker")})) + + view := nm.View() + if !strings.Contains(view, "in-overlay-marker") { + t.Error("View() does not show the tool overlay's content") + } + if strings.Contains(view, "github token") { + t.Error("View() fell through to the API-status overlay") + } +} + +var _ tea.Model = Model{} diff --git a/internal/model/render.go b/internal/model/render.go index 1569a1e..1c04e66 100644 --- a/internal/model/render.go +++ b/internal/model/render.go @@ -30,9 +30,18 @@ func (m Model) View() string { body := lipgloss.JoinHorizontal(lipgloss.Top, left, middle, right) layout := lipgloss.JoinVertical(lipgloss.Left, body, m.renderStatusBar()) if m.overlayVisible() { - fg := m.renderAPIStatus() - if m.mode == modeHotkeys { + // One picker per overlay mode. It is a switch rather than the two-way + // if it grew out of because the API-status default silently answered + // for every mode that was not modeHotkeys — with a third overlay that + // would have painted the token panel over a running tool. + var fg string + switch m.mode { + case modeHotkeys: fg = m.renderHotkeys() + case modeToolOverlay: + fg = m.renderToolOverlay() + default: + fg = m.renderAPIStatus() } layout = ui.PlaceOverlay(layout, fg, m.sty().OverlayDim) } diff --git a/internal/model/zoom_test.go b/internal/model/zoom_test.go index dd4c49b..27038d0 100644 --- a/internal/model/zoom_test.go +++ b/internal/model/zoom_test.go @@ -284,7 +284,7 @@ func TestZoomInSearchStaysQueryText(t *testing.T) { // screen underneath an open modal is the one failure this would produce, and // modeSearch next door proves the dispatch is not uniform. func TestZoomUnderOverlay(t *testing.T) { - for _, mode := range []inputMode{modeHotkeys, modeAPIStatus} { + for _, mode := range []inputMode{modeHotkeys, modeAPIStatus, modeToolOverlay} { m := zoomModel(t, 160, 40) m.mode = mode nm := mustModel(m.Update(keyRunes("z"))) From e7422510b9cf24f0fc9bd0872f6b19035ddb19c5 Mon Sep 17 00:00:00 2001 From: stanlyzoolo <51911715+stanlyzoolo@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:35:16 +0300 Subject: [PATCH 04/13] feat: route the keyboard to the tool in the overlay While the tool runs every key is translated and sent, esc and ctrl+c included: esc is what makes vim usable and ctrl+c is the tool's interrupt, so neither may mean anything to keepkit. ctrl+\ is the one reserved chord and it kills without leaving the mode, because the outcome line the user killed something to read arrives with the exit. Translation splits by what actually depends on emulator state: the mode-dependent named keys go through SendKey so DECCKM is honoured, and the control range is written as the byte itself, which is what Bubble Tea's key types already are and what vt's encoder would have produced. The modified cursor keys are hand-encoded as xterm CSI 1; - the pinned x/vt emits nothing at all for them, which would have swallowed ctrl+left in every editor. Co-Authored-By: Claude Opus 5 (1M context) --- docs/plans/20260814-tool-overlay-terminal.md | 17 +- internal/model/overlay_term.go | 144 ++++++++++++++- internal/model/overlay_term_test.go | 185 +++++++++++++++++++ 3 files changed, 337 insertions(+), 9 deletions(-) diff --git a/docs/plans/20260814-tool-overlay-terminal.md b/docs/plans/20260814-tool-overlay-terminal.md index 72a20b9..fca6146 100644 --- a/docs/plans/20260814-tool-overlay-terminal.md +++ b/docs/plans/20260814-tool-overlay-terminal.md @@ -133,12 +133,17 @@ - Modify: `internal/model/mode.go`, `internal/model/overlay_term.go` - Modify: `internal/model/overlay_term_test.go` -- [ ] implement key translation `tea.KeyMsg` → tool input (runes → text; named keys/ctrl-chords → key events; mechanism per Task 1's decision), confirmed against the pinned vt version; esc translates and is **sent**, not consumed, while the process runs -- [ ] add `case modeToolOverlay: return m.updateToolOverlay(msg)` to the mode dispatch **unwrapped** — deliberately *not* through `flushPendingLaunch` like its siblings: a deferred exec fallback must not fire `tea.ExecProcess` on the very keystroke that closes the overlay (the wrapper disappears entirely in Task 5) -- [ ] `updateToolOverlay`: process alive → translate & send everything except `ctrl+\` (→ `Session.Kill`, stays in mode until `termExitMsg`); process exited → `esc` cleans up (session close, `termEmu = nil`, `modeNormal`), everything else no-op -- [ ] **pre-`termStartedMsg` nil guard**: keys arriving before the session exists are dropped, `ctrl+\` is a no-op there (a nil deref would re-panic through `logx.Recover` and crash keepkit) -- [ ] write tests: keys (incl. esc, ctrl+c) reach the fake's input while alive; `ctrl+\` triggers `Kill` and mode holds; esc before exit does NOT close; esc after `termExitMsg` cleans and returns to `modeNormal`; non-esc after exit is a no-op; a key in the pre-started state neither panics nor reaches anything -- [ ] run the full gate (build + vet + `go test -race ./...` + lint) - must pass before task 4 +- [x] implement key translation `tea.KeyMsg` → tool input (runes → text; named keys/ctrl-chords → key events; mechanism per Task 1's decision), confirmed against the pinned vt version; esc translates and is **sent**, not consumed, while the process runs + - three paths, split by what actually depends on emulator state: **runes/space** → `SendText`; **the mode-dependent named keys** (arrows, Home/End, PgUp/PgDn, Insert/Delete, shift+tab, F1–F12) → `SendKey`, so DECCKM/DECNKM are honoured; **the control range** → the byte itself, because Bubble Tea's control key *types are* the control bytes (`KeyEnter` is 0x0d, `KeyEsc` 0x1b, `KeyBackspace` 0x7f) and none of them is mode-dependent — writing the byte is exactly what vt's encoder produces. + - ➕ **`termModifiedKeys`**: the pinned x/vt encodes no modified special key at all — its `SendKey` default emits nothing when `Mod != 0` — so `ctrl+left` and friends would be silently swallowed in every editor. They are written out as xterm's `CSI 1;`, which is safe to hand-encode precisely because, unlike the bare arrows, the modified forms do not depend on DECCKM. + - `Alt` is a Bubble Tea *flag*, not a key: it becomes an ESC prefix (or `ModAlt` on the `SendKey` path). +- [x] add `case modeToolOverlay: return m.updateToolOverlay(msg)` to the mode dispatch **unwrapped** — deliberately *not* through `flushPendingLaunch` like its siblings: a deferred exec fallback must not fire `tea.ExecProcess` on the very keystroke that closes the overlay (the wrapper disappears entirely in Task 5) + - landed in Task 2 (see the ⚠️ ordering note there); `TestToolOverlayDoesNotFlushPendingLaunch` is what pins the unwrapping. +- [x] `updateToolOverlay`: process alive → translate & send everything except `ctrl+\` (→ `Session.Kill`, stays in mode until `termExitMsg`); process exited → `esc` cleans up (session close, `termEmu = nil`, `modeNormal`), everything else no-op +- [x] **pre-`termStartedMsg` nil guard**: keys arriving before the session exists are dropped, `ctrl+\` is a no-op there (a nil deref would re-panic through `logx.Recover` and crash keepkit) +- [x] write tests: keys (incl. esc, ctrl+c) reach the fake's input while alive; `ctrl+\` triggers `Kill` and mode holds; esc before exit does NOT close; esc after `termExitMsg` cleans and returns to `modeNormal`; non-esc after exit is a no-op; a key in the pre-started state neither panics nor reaches anything + - ➕ the forwarding test is a **19-row table asserting the exact bytes** the tool receives, not just that something arrived: the encoding is the feature, and "a key reached the fake" would pass with every sequence wrong. +- [x] run the full gate (build + vet + `go test -race ./...` + lint) - must pass before task 4 ### Task 4: rendering — geometry, overlay frame, cursor, exit line, status bar, resize diff --git a/internal/model/overlay_term.go b/internal/model/overlay_term.go index f0e7673..2e8f081 100644 --- a/internal/model/overlay_term.go +++ b/internal/model/overlay_term.go @@ -5,6 +5,7 @@ import ( "time" tea "github.com/charmbracelet/bubbletea" + uv "github.com/charmbracelet/ultraviolet" "github.com/charmbracelet/x/vt" "github.com/stanlyzoolo/keepkit/internal/term" @@ -262,15 +263,152 @@ func (m *Model) closeToolOverlay() { m.mode = modeNormal } +// termKillKey is the one chord the overlay keeps for itself. Everything else — +// esc, ctrl+c, q — belongs to the tool, so the way out of a hung program cannot +// be a key the program might want. ctrl+\ is the choice because it is already +// SIGQUIT by convention and no editor binds it. +const termKillKey = tea.KeyCtrlBackslash + // updateToolOverlay owns every keystroke while the overlay is open — that is // what "the tool has the keyboard" means, and it is why the mode consumes keys // it has no use for rather than letting them fall through to the normal-mode -// map underneath. Key translation, the ctrl+\ kill chord and esc-after-exit -// land in Task 3 of the tool-overlay plan. -func (m Model) updateToolOverlay(_ tea.KeyMsg) (tea.Model, tea.Cmd) { +// map underneath. +// +// While the tool runs, every key is translated and sent, ctrl+\ kills. Once it +// has exited the overlay is a still image: esc closes it and nothing else does +// anything — in particular the keys are no longer forwarded, because there is +// nothing left to forward them to. +func (m Model) updateToolOverlay(msg tea.KeyMsg) (tea.Model, tea.Cmd) { + if m.termExit != nil { + if msg.Type == tea.KeyEsc { + m.closeToolOverlay() + } + return m, nil + } + + if msg.Type == termKillKey { + // Kill, but stay in the mode: the verdict arrives as termExitMsg and + // that is what turns the overlay into its outcome line. Leaving here + // would drop the log the user just killed something to read. + if m.termSession != nil { + m.termSession.Kill() + } + return m, nil + } + + // Between the keypress that opened the overlay and termStartedMsg there is + // no emulator and no session. Keys are dropped rather than queued: the + // window is milliseconds, and a nil deref here would re-panic through + // logx.Recover and take keepkit down with it. + if m.termEmu != nil { + sendToolKey(m.termEmu, msg) + } return m, nil } +// termNamedKeys maps the Bubble Tea key types that need the emulator's own +// encoder — the ones whose bytes depend on emulator state (DECCKM turns the +// arrows and Home/End into their \x1bO… forms, DECNKM the keypad). Everything +// here goes through SendKey for that reason; see the file header on why the +// model cannot encode them itself. +var termNamedKeys = map[tea.KeyType]uv.KeyPressEvent{ + tea.KeyUp: {Code: uv.KeyUp}, + tea.KeyDown: {Code: uv.KeyDown}, + tea.KeyRight: {Code: uv.KeyRight}, + tea.KeyLeft: {Code: uv.KeyLeft}, + tea.KeyHome: {Code: uv.KeyHome}, + tea.KeyEnd: {Code: uv.KeyEnd}, + tea.KeyPgUp: {Code: uv.KeyPgUp}, + tea.KeyPgDown: {Code: uv.KeyPgDown}, + tea.KeyInsert: {Code: uv.KeyInsert}, + tea.KeyDelete: {Code: uv.KeyDelete}, + tea.KeyShiftTab: {Code: uv.KeyTab, Mod: uv.ModShift}, + tea.KeyF1: {Code: uv.KeyF1}, + tea.KeyF2: {Code: uv.KeyF2}, + tea.KeyF3: {Code: uv.KeyF3}, + tea.KeyF4: {Code: uv.KeyF4}, + tea.KeyF5: {Code: uv.KeyF5}, + tea.KeyF6: {Code: uv.KeyF6}, + tea.KeyF7: {Code: uv.KeyF7}, + tea.KeyF8: {Code: uv.KeyF8}, + tea.KeyF9: {Code: uv.KeyF9}, + tea.KeyF10: {Code: uv.KeyF10}, + tea.KeyF11: {Code: uv.KeyF11}, + tea.KeyF12: {Code: uv.KeyF12}, +} + +// termModifiedKeys covers the modified cursor keys, which the pinned x/vt does +// not encode at all — its SendKey falls through to a default that emits nothing +// for a modified special key, so routing these through the emulator would +// silently swallow ctrl+left in every editor. +// +// They are safe to write out here because, unlike the bare arrows, the modified +// forms do not depend on DECCKM: xterm's CSI 1; shape is what every +// terminal sends whatever the cursor-keys mode. The modifier digit is +// 1+shift(1)+alt(2)+ctrl(4), and the finals are A/B/C/D for the arrows and H/F +// for home/end. +var termModifiedKeys = map[tea.KeyType]string{ + tea.KeyShiftUp: "\x1b[1;2A", + tea.KeyShiftDown: "\x1b[1;2B", + tea.KeyShiftRight: "\x1b[1;2C", + tea.KeyShiftLeft: "\x1b[1;2D", + tea.KeyShiftHome: "\x1b[1;2H", + tea.KeyShiftEnd: "\x1b[1;2F", + tea.KeyCtrlUp: "\x1b[1;5A", + tea.KeyCtrlDown: "\x1b[1;5B", + tea.KeyCtrlRight: "\x1b[1;5C", + tea.KeyCtrlLeft: "\x1b[1;5D", + tea.KeyCtrlHome: "\x1b[1;5H", + tea.KeyCtrlEnd: "\x1b[1;5F", + tea.KeyCtrlShiftUp: "\x1b[1;6A", + tea.KeyCtrlShiftDown: "\x1b[1;6B", + tea.KeyCtrlShiftRight: "\x1b[1;6C", + tea.KeyCtrlShiftLeft: "\x1b[1;6D", + tea.KeyCtrlShiftHome: "\x1b[1;6H", + tea.KeyCtrlShiftEnd: "\x1b[1;6F", + tea.KeyCtrlPgUp: "\x1b[5;5~", + tea.KeyCtrlPgDown: "\x1b[6;5~", +} + +// sendToolKey translates one Bubble Tea key into what the tool should read. +// +// Bubble Tea's control-key types are the control bytes themselves — KeyEnter is +// 0x0d, KeyEsc 0x1b, KeyCtrlA 0x01, KeyBackspace 0x7f — and none of those is +// mode-dependent, so writing the byte is exactly what the emulator's encoder +// would have produced for them. Alt arrives as a flag rather than a key, and an +// alt-modified key is its unmodified form behind an ESC. +func sendToolKey(emu *vt.Emulator, msg tea.KeyMsg) { + alt := "" + if msg.Alt { + alt = "\x1b" + } + + switch msg.Type { + case tea.KeyRunes: + emu.SendText(alt + string(msg.Runes)) + case tea.KeySpace: + emu.SendText(alt + " ") + default: + if seq, ok := termModifiedKeys[msg.Type]; ok { + emu.SendText(alt + seq) + return + } + if key, ok := termNamedKeys[msg.Type]; ok { + if msg.Alt { + key.Mod |= uv.ModAlt + } + emu.SendKey(key) + return + } + // What is left is the control range, where the key type *is* the byte. + // A type outside it is one Bubble Tea knows and this build does not + // (a newer F-key, say); dropping it beats sending a stray rune. + if msg.Type >= 0 && msg.Type <= 0x7f { + emu.SendText(alt + string(rune(msg.Type))) + } + } +} + // renderToolOverlay draws the embedded terminal. Placeholder: the frame, // geometry, cursor and outcome line land in Task 4 of the tool-overlay plan. func (m Model) renderToolOverlay() string { diff --git a/internal/model/overlay_term_test.go b/internal/model/overlay_term_test.go index 0e469e6..58cca8b 100644 --- a/internal/model/overlay_term_test.go +++ b/internal/model/overlay_term_test.go @@ -380,4 +380,189 @@ func TestViewPicksTheToolOverlay(t *testing.T) { } } +// Every key reaches the tool while it runs — including the ones keepkit would +// otherwise act on. esc must not close the overlay (vim needs it) and ctrl+c +// must not quit keepkit (it is the tool's interrupt), which is the whole claim +// "the tool has the keyboard" makes. +func TestToolOverlayForwardsEveryKeyWhileRunning(t *testing.T) { + tests := []struct { + name string + key tea.KeyMsg + want string + }{ + {"letters", keyRunes("ls"), "ls"}, + {"space", tea.KeyMsg{Type: tea.KeySpace}, " "}, + {"enter", tea.KeyMsg{Type: tea.KeyEnter}, "\r"}, + {"tab", tea.KeyMsg{Type: tea.KeyTab}, "\t"}, + {"backspace", tea.KeyMsg{Type: tea.KeyBackspace}, "\x7f"}, + {"esc goes to the tool", tea.KeyMsg{Type: tea.KeyEsc}, "\x1b"}, + {"ctrl+c goes to the tool", tea.KeyMsg{Type: tea.KeyCtrlC}, "\x03"}, + {"ctrl+d", tea.KeyMsg{Type: tea.KeyCtrlD}, "\x04"}, + {"up arrow", tea.KeyMsg{Type: tea.KeyUp}, "\x1b[A"}, + {"down arrow", tea.KeyMsg{Type: tea.KeyDown}, "\x1b[B"}, + {"home", tea.KeyMsg{Type: tea.KeyHome}, "\x1b[H"}, + {"page up", tea.KeyMsg{Type: tea.KeyPgUp}, "\x1b[5~"}, + {"delete", tea.KeyMsg{Type: tea.KeyDelete}, "\x1b[3~"}, + {"shift+tab", tea.KeyMsg{Type: tea.KeyShiftTab}, "\x1b[Z"}, + {"f1", tea.KeyMsg{Type: tea.KeyF1}, "\x1bOP"}, + {"f5", tea.KeyMsg{Type: tea.KeyF5}, "\x1b[15~"}, + {"ctrl+left", tea.KeyMsg{Type: tea.KeyCtrlLeft}, "\x1b[1;5D"}, + {"shift+right", tea.KeyMsg{Type: tea.KeyShiftRight}, "\x1b[1;2C"}, + {"alt+rune", tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("b"), Alt: true}, "\x1bb"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := newFakeSession(4) + m := overlayModel(t, f) + + nm := mustModel(m.Update(tt.key)) + + if got := f.input(t, tt.want); !strings.Contains(got, tt.want) { + t.Errorf("tool received %q, want it to contain %q", got, tt.want) + } + if nm.mode != modeToolOverlay { + t.Errorf("mode = %v, want the overlay to keep the keyboard", nm.mode) + } + }) + } +} + +// esc while the tool runs is the tool's, not the overlay's: closing here would +// make vim unusable, which is the case the whole design is built around. +func TestToolOverlayEscDoesNotCloseWhileRunning(t *testing.T) { + f := newFakeSession(4) + m := overlayModel(t, f) + + nm := mustModel(m.Update(tea.KeyMsg{Type: tea.KeyEsc})) + + if nm.mode != modeToolOverlay { + t.Errorf("mode = %v after esc, want the overlay still open", nm.mode) + } + if nm.termSession == nil { + t.Error("esc tore down a running session") + } +} + +// ctrl+c must not reach keepkit's global quit. That case sits after the mode +// dispatch, so this is structural - asserted because the failure is keepkit +// exiting out from under a running tool. +func TestToolOverlayCtrlCDoesNotQuit(t *testing.T) { + f := newFakeSession(4) + m := overlayModel(t, f) + + nm, cmd := m.Update(tea.KeyMsg{Type: tea.KeyCtrlC}) + + if cmd != nil { + t.Error("ctrl+c in the overlay returned a command (tea.Quit reached)") + } + if nm.(Model).mode != modeToolOverlay { + t.Error("ctrl+c left the overlay") + } +} + +// ctrl+\ is the reserved chord: it kills but stays in the mode, because the +// outcome line the user killed something to read arrives with termExitMsg. +func TestToolOverlayKillChord(t *testing.T) { + f := newFakeSession(4) + m := overlayModel(t, f) + + nm := mustModel(m.Update(tea.KeyMsg{Type: tea.KeyCtrlBackslash})) + + killed, _ := f.counts() + if killed != 1 { + t.Errorf("Kill called %d times, want 1", killed) + } + if nm.mode != modeToolOverlay { + t.Errorf("mode = %v after ctrl+\\, want to stay until the exit arrives", nm.mode) + } + if nm.termExit != nil { + t.Error("ctrl+\\ synthesised an exit instead of waiting for the session's") + } + if got := f.input(t, ""); strings.Contains(got, "\x1c") { + t.Error("the kill chord was also forwarded to the tool") + } +} + +// After the exit the overlay is a still image: esc closes, everything else does +// nothing, and nothing is forwarded because there is nothing to forward to. +func TestToolOverlayAfterExit(t *testing.T) { + f := newFakeSession(4) + base := overlayModel(t, f) + base = mustModel(base.Update(termExitMsg{elapsed: time.Second})) + + t.Run("esc closes", func(t *testing.T) { + m := base + nm := mustModel(m.Update(tea.KeyMsg{Type: tea.KeyEsc})) + if nm.mode != modeNormal { + t.Errorf("mode = %v after esc, want modeNormal", nm.mode) + } + if nm.termSession != nil || nm.termEmu != nil || nm.termExit != nil { + t.Error("esc left overlay state behind") + } + }) + + t.Run("other keys are no-ops", func(t *testing.T) { + for _, key := range []tea.KeyMsg{ + keyRunes("q"), tea.KeyMsg{Type: tea.KeyEnter}, tea.KeyMsg{Type: tea.KeyCtrlC}, + tea.KeyMsg{Type: tea.KeyCtrlBackslash}, + } { + m := base + nm, cmd := m.Update(key) + if nm.(Model).mode != modeToolOverlay { + t.Errorf("key %v left the exited overlay", key) + } + if cmd != nil { + t.Errorf("key %v returned a command from an exited overlay", key) + } + } + // nothing was written to a tool that is no longer there + if got := f.input(t, ""); got != "" { + t.Errorf("an exited overlay forwarded %q to the dead session", got) + } + }) +} + +// The window between the keypress and termStartedMsg has no session and no +// emulator. A key there must be dropped, not dereferenced: a panic would +// re-panic through logx.Recover and take keepkit down. +func TestToolOverlayKeysBeforeStart(t *testing.T) { + m := newTestModel(focusTools) + m.mode = modeToolOverlay + m.termToolName = "git" + + for _, key := range []tea.KeyMsg{ + keyRunes("x"), tea.KeyMsg{Type: tea.KeyEsc}, tea.KeyMsg{Type: tea.KeyCtrlBackslash}, + tea.KeyMsg{Type: tea.KeyUp}, + } { + nm, cmd := m.Update(key) // must not panic + if nm.(Model).mode != modeToolOverlay { + t.Errorf("key %v left the pre-start overlay", key) + } + if cmd != nil { + t.Errorf("key %v returned a command before the session existed", key) + } + } +} + +// The overlay's dispatch is deliberately not wrapped in flushPendingLaunch, +// unlike every sibling mode: a deferred exec fallback firing tea.ExecProcess on +// the keystroke that closes the overlay would seize the terminal. +func TestToolOverlayDoesNotFlushPendingLaunch(t *testing.T) { + f := newFakeSession(4) + m := overlayModel(t, f) + m.pendingLaunchName = "fd" + m.pendingLaunchCommand = "fd --version" + m = mustModel(m.Update(termExitMsg{elapsed: time.Second})) + + nm, cmd := m.Update(tea.KeyMsg{Type: tea.KeyEsc}) + + if cmd != nil { + t.Error("closing the overlay dispatched the deferred exec fallback") + } + if nm.(Model).pendingLaunchName != "fd" { + t.Error("the deferred fallback was consumed by the overlay's dispatch") + } +} + var _ tea.Model = Model{} From 7688a9af3419b98a7eb00942fc113141c1620576 Mon Sep 17 00:00:00 2001 From: stanlyzoolo <51911715+stanlyzoolo@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:46:15 +0300 Subject: [PATCH 05/13] feat: render the tool overlay - frame, cursor and outcome line The block is 70% of the screen with the exit row reserved from the start, so its height never changes when the tool finishes. Looking at it at 80 columns caught what the arithmetic missed: nothing held the *width*, and lipgloss sizes a border to its widest line, so the block measured 54 cells running and 53 exited and jumped sideways at the exact moment the user starts reading the outcome. Every row is now padded to the body width, ANSI-safely in both directions. The child's cursor is a reverse-video cell spliced by visible column through x/ansi - a rune-index cut would land inside an escape sequence that the terminal then executes. It is hidden once the tool has exited, because a cursor on a dead screen invites typing. Geometry returns the size and the verdict separately: the keypress refuses when the screen is too small, but a terminal shrunk mid-session clamps to the floor and keeps the tool running. Killing somebody's editor because they narrowed a window is worse than a cramped overlay. Every new assertion was mutation-checked. Two survived at first and both were test defects: the block-size helper was measuring the panels' borders rather than the overlay's, and the width claim was already satisfied by the body rows, so it now asserts the case the clamp exists for - a tool name longer than the body. Co-Authored-By: Claude Opus 5 (1M context) --- docs/plans/20260814-tool-overlay-terminal.md | 25 +- internal/model/model.go | 4 + internal/model/overlay_term.go | 242 +++++++++++- internal/model/overlay_term_test.go | 381 +++++++++++++++++++ internal/model/render.go | 10 + internal/ui/styles.go | 9 + 6 files changed, 657 insertions(+), 14 deletions(-) diff --git a/docs/plans/20260814-tool-overlay-terminal.md b/docs/plans/20260814-tool-overlay-terminal.md index fca6146..28a2bfa 100644 --- a/docs/plans/20260814-tool-overlay-terminal.md +++ b/docs/plans/20260814-tool-overlay-terminal.md @@ -151,14 +151,23 @@ - Modify: `internal/model/render.go`, `internal/model/overlay_term.go`, `internal/model/model.go` (`applyLayout`/`WindowSizeMsg`) - Modify: `internal/model/overlay_term_test.go` (and/or `render_test.go`) -- [ ] implement `termGeometry(width, height)` beside the frame arithmetic it must agree with: outer block = 70%×70% centered; body `emuW = outerW - 4` (border + `Padding(0,1)`), `emuH = outerH - 2 - 1 (title) - 1 (reserved exit row)`; account for `View()`'s `Margin(1, 0)` and PlaceOverlay's silent bottom clip; min clamps ≈40×10 body; ok=false below them **and when `!m.ready` (checked first, the `toggleZoom` idiom)** -- [ ] render the overlay in the `View()` switch: `OverlayBorder` frame, title = tool name + dim `ctrl+\ kill` right-hint while running; body = `termEmu.Render()` (pre-start: `starting …`); the **exit row is reserved from the start** (blank while running) so the block height is constant and the overlay never re-centers at exit -- [ ] render the child's cursor: reverse-video the cell at the emulator's cursor position while the process is alive, hidden after exit; honour the emulator's cursor-visibility state if the API exposes it -- [ ] fill the reserved row after exit via `updateOutcomeBlock`'s helpers (`formatElapsed`/`fitCells`/`footerSep`): `✓ exited · · esc close` / `✕ exit · · esc close` / `✕ killed · · esc close` / `✕ failed to start · · esc close` (`✓`/`✕` in `Ok`/`Danger`) -- [ ] add the `renderStatusBar` branch for `modeToolOverlay` (its siblings all have one — without it the bar advertises six dead global keys): running → ` running ctrl+\ kill`, exited → ` exited esc close`, within the no-truncate budget -- [ ] handle `WindowSizeMsg` while open: recompute `termGeometry`, `termEmu.Resize` + `Session.Resize`; shrinking below the minimum keeps the overlay at the clamped floor (no mid-session kill) -- [ ] write tests: rendered View contains frame, title, hint, `starting…` body pre-start, cursor cell reverse-video while alive, all four outcome lines after their exits; **block height identical before and after `termExitMsg`**; dimmed background; a body line carrying SGR renders at the right visible width inside the frame and no escape leaks past the frame's right edge (assert on `stripANSI(View())` width and the dim margins); status-bar branch in both states; resize propagates to fake session and emulator; refusal on a tiny terminal and on `!m.ready` -- [ ] run the full gate (build + vet + `go test -race ./...` + lint) - must pass before task 5 +- [x] implement `termGeometry(width, height)` beside the frame arithmetic it must agree with: outer block = 70%×70% centered; body `emuW = outerW - 4` (border + `Padding(0,1)`), `emuH = outerH - 2 - 1 (title) - 1 (reserved exit row)`; account for `View()`'s `Margin(1, 0)` and PlaceOverlay's silent bottom clip; min clamps ≈40×10 body; ok=false below them **and when `!m.ready` (checked first, the `toggleZoom` idiom)** + - ➕ the size and the verdict are **separate answers**: the size is always clamped to the floor and `ok` is returned beside it, because the two callers want different things — the keypress refuses on `!ok`, while a terminal shrunk *mid-session* keeps rendering at the minimum. Killing somebody's editor because they narrowed a window is worse than a cramped overlay. + - at the 80×24 baseline the body is **52×11** — the default terminal succeeds, which is the number that mattered. +- [x] render the overlay in the `View()` switch: `OverlayBorder` frame, title = tool name + dim `ctrl+\ kill` right-hint while running; body = `termEmu.Render()` (pre-start: `starting …`); the **exit row is reserved from the start** (blank while running) so the block height is constant and the overlay never re-centers at exit + - ⚠️ **the visual pass caught what the plan's arithmetic missed**: the reserved row holds the *height*, but nothing held the *width*, and lipgloss sizes a border to its widest line. At 80×24 the block measured 54 cells running and 53 exited — it jumped sideways at the exact moment the user starts reading the outcome. Fixed by padding every row to the body width (`termRow`, ANSI-safe in both directions). +- [x] render the child's cursor: reverse-video the cell at the emulator's cursor position while the process is alive, hidden after exit; honour the emulator's cursor-visibility state if the API exposes it + - the pinned x/vt exposes `CursorPosition()` but **no visibility accessor**, so the alive/exited rule is the whole of it. The splice goes through `ansi.Truncate`/`TruncateLeft` (cut by *visible column*, keeping SGR on both sides) — a rune-index cut would land inside an escape sequence. + - ➕ `ui.Styles.TermCursor` is a new style and the only one naming no theme colour: reverse video swaps whatever the *tool* painted, which is the only way one style can mark a cursor on a screen keepkit does not control. +- [x] fill the reserved row after exit via `updateOutcomeBlock`'s helpers (`formatElapsed`/`fitCells`/`footerSep`): `✓ exited · · esc close` / `✕ exit · · esc close` / `✕ killed · · esc close` / `✕ failed to start · · esc close` (`✓`/`✕` in `Ok`/`Danger`) + - a start failure spends the middle cell on its **reason** rather than on an elapsed it does not have. +- [x] add the `renderStatusBar` branch for `modeToolOverlay` (its siblings all have one — without it the bar advertises six dead global keys): running → ` running ctrl+\ kill`, exited → ` exited esc close`, within the no-truncate budget +- [x] handle `WindowSizeMsg` while open: recompute `termGeometry`, `termEmu.Resize` + `Session.Resize`; shrinking below the minimum keeps the overlay at the clamped floor (no mid-session kill) + - hung off **`applyLayout`**, the single relayout point, so the resize path cannot drift from it; a no-op in every other mode. +- [x] write tests: rendered View contains frame, title, hint, `starting…` body pre-start, cursor cell reverse-video while alive, all four outcome lines after their exits; **block height identical before and after `termExitMsg`**; dimmed background; a body line carrying SGR renders at the right visible width inside the frame and no escape leaks past the frame's right edge (assert on `stripANSI(View())` width and the dim margins); status-bar branch in both states; resize propagates to fake session and emulator; refusal on a tiny terminal and on `!m.ready` + - **every new assertion was mutation-checked** (11 mutations, all killed). Two survived on the first pass and both were real test defects, not code ones: the block-size helper was scanning the *composited* view, where the three panels' own rounded corners are what it found, so it reported a constant size no matter what the overlay did; and the width claim was already satisfied by the body rows, so it was rewritten to assert the case the clamp actually exists for — **a tool name longer than the body must not widen the block**. + - the cursor test needs `forceColor(t)`: reverse video is styling, and the default test profile strips it, so the assertion would have passed against a cursor that was never drawn. +- [x] run the full gate (build + vet + `go test -race ./...` + lint) - must pass before task 5 ### Task 5: the switch — dispatch to overlay, delete the tab launcher wholesale diff --git a/internal/model/model.go b/internal/model/model.go index dcddfd7..559d4e5 100644 --- a/internal/model/model.go +++ b/internal/model/model.go @@ -2384,6 +2384,10 @@ func (m *Model) applyLayout() { // the pre-resize content (wrong scroll, out-of-range click mapping). m.setToolsContent() m.briefViewport.SetContent(m.renderCard()) + // The embedded terminal is sized off the screen rather than off the panel + // widths, so it follows a resize from here — the single relayout point — + // and is a no-op in every other mode. + m.resizeToolOverlay() } // initViewports creates the three viewports on the first relayout — the diff --git a/internal/model/overlay_term.go b/internal/model/overlay_term.go index 2e8f081..f911120 100644 --- a/internal/model/overlay_term.go +++ b/internal/model/overlay_term.go @@ -1,11 +1,17 @@ package model import ( + "errors" "io" + "os/exec" + "strconv" + "strings" "time" tea "github.com/charmbracelet/bubbletea" + "github.com/charmbracelet/lipgloss" uv "github.com/charmbracelet/ultraviolet" + "github.com/charmbracelet/x/ansi" "github.com/charmbracelet/x/vt" "github.com/stanlyzoolo/keepkit/internal/term" @@ -409,14 +415,238 @@ func sendToolKey(emu *vt.Emulator, msg tea.KeyMsg) { } } -// renderToolOverlay draws the embedded terminal. Placeholder: the frame, -// geometry, cursor and outcome line land in Task 4 of the tool-overlay plan. -func (m Model) renderToolOverlay() string { - body := "starting " + m.termToolName + "…" +// Overlay geometry. The block covers termOverlayPercent of the screen in both +// directions; the minimums are what a tool actually needs to be usable rather +// than merely drawn — below them the keypress refuses instead of opening a +// window nothing fits in. +const ( + termOverlayPercent = 70 + termMinBodyW = 40 + termMinBodyH = 10 + + // termChromeRows is what the frame spends on itself inside the border: the + // title row and the exit row. The exit row is reserved from the very start + // and left blank while the tool runs, so the block's height never changes + // and PlaceOverlay cannot re-centre it the moment the tool exits. + termChromeRows = 2 + // termChromeCols is the border (2) plus OverlayBorder's Padding(0, 1) (2). + termChromeCols = 4 +) + +// termGeometry is the emulator's body size for the current terminal, plus +// whether the screen is big enough for one at all. +// +// The size is always clamped to the floor and ok is a separate answer, because +// the two callers want different things: the keypress refuses on !ok, while a +// terminal shrunk *mid-session* keeps rendering at the minimum — resizing the +// user's editor into nothing is bad, killing it outright is worse. +// +// !m.ready is checked first and explicitly, the toggleZoom idiom: before the +// first WindowSizeMsg every dimension is zero and the percentages below would +// agree on a floor-sized overlay by coincidence rather than by measurement. +func (m Model) termGeometry() (w, h int, ok bool) { + if !m.ready { + return termMinBodyW, termMinBodyH, false + } + return termGeometryFor(m.width, m.height) +} + +// termGeometryFor is the pure core (the planFor/baseFor idiom), so the clamps +// are testable without a laid-out model. +func termGeometryFor(width, height int) (w, h int, ok bool) { + // The overlay composites over the layout, not over the terminal: View wraps + // the composite in Margin(1, 0), so two rows are already spoken for and an + // overlay sized against the raw height would be clipped at the bottom by + // PlaceOverlay — silently, which is how a missing exit line would look. + bgH := height - 2 + + outerW := min(width*termOverlayPercent/100, width) + outerH := min(bgH*termOverlayPercent/100, bgH) + + w, h = outerW-termChromeCols, outerH-2-termChromeRows + ok = w >= termMinBodyW && h >= termMinBodyH + return max(w, termMinBodyW), max(h, termMinBodyH), ok +} + +// resizeToolOverlay follows a terminal resize through to the tool: the emulator +// is re-laid-out first and the pty is told second, so the child's redraw lands +// on a screen that is already the right shape. +func (m *Model) resizeToolOverlay() { + if m.mode != modeToolOverlay { + return + } + w, h, _ := m.termGeometry() + if w == m.termW && h == m.termH { + return + } + m.termW, m.termH = w, h if m.termEmu != nil { - body = m.termEmu.Render() + m.termEmu.Resize(w, h) + } + if m.termSession != nil { + _ = m.termSession.Resize(w, h) + } +} + +// renderToolOverlay draws the embedded terminal: a title row naming the tool, +// the tool's own screen, and a bottom row that is blank while it runs and +// carries the verdict once it has exited. +func (m Model) renderToolOverlay() string { + s := m.sty() + w, h, _ := m.termGeometry() + + // Title. The kill chord is advertised here rather than only in the status + // bar because this frame is what the user is looking at, and it is the one + // key that still belongs to keepkit while the tool has the keyboard. It + // goes once the tool has exited — by then the only key that does anything + // is esc, and the exit row says so. + title := s.EmphasisBold.Render(m.termToolName) + titleRow := title + if m.termExit == nil { + hint := m.hint(`ctrl+\`, "kill") + if pad := w - lipgloss.Width(title) - lipgloss.Width(hint); pad > 0 { + titleRow = title + strings.Repeat(" ", pad) + hint + } + } + + rows := make([]string, 0, h+termChromeRows) + rows = append(rows, titleRow) + rows = append(rows, m.termBodyRows(w, h)...) + rows = append(rows, m.termExitRow(w)) + + // Every row is padded to exactly w before the frame goes round it. lipgloss + // sizes a border to its widest line, so without this the block would be as + // wide as whatever the tool happened to print — and, worse, it would *change + // width at exit*, when the title loses its kill hint and the exit row + // replaces a blank. PlaceOverlay centres what it is given, so a one-cell + // change is the whole block jumping sideways at the moment the user is + // reading the outcome. The reserved row keeps the height constant; this + // keeps the width constant. + for i, row := range rows { + rows[i] = termRow(row, w) + } + return s.OverlayBorder.Render(strings.Join(rows, "\n")) +} + +// termRow fits one row to exactly w visible cells. +// +// Both directions go through x/ansi: padding is plain spaces appended after the +// styling (never inside it), and a row too wide is cut by visible column rather +// than by byte or rune index, which would land inside an escape sequence that +// the terminal then executes. +func termRow(row string, w int) string { + switch width := lipgloss.Width(row); { + case width < w: + return row + strings.Repeat(" ", w-width) + case width > w: + return ansi.Truncate(row, w, "") + default: + return row + } +} + +// termBodyRows is the tool's screen, exactly h rows tall. +// +// Before termStartedMsg there is no emulator: the overlay is already on screen +// (the keypress opened it) and has to say something, which is the same +// "starting …" idiom the update log uses for its own pre-output window. +func (m Model) termBodyRows(w, h int) []string { + rows := make([]string, h) + if m.termEmu == nil { + rows[0] = m.sty().Dim.Render("starting " + m.termToolName + "…") + return rows + } + + lines := strings.Split(m.termEmu.Render(), "\n") + for i := range rows { + if i < len(lines) { + // Padded here rather than by the final pass, because the cursor + // splice below addresses a *column*: on a short line, a cursor + // past its end would otherwise be spliced in right after the last + // glyph instead of where the tool actually put it. + rows[i] = termRow(lines[i], w) + } else { + rows[i] = strings.Repeat(" ", w) + } + } + return m.withTermCursor(rows, w, h) +} + +// withTermCursor reverse-videos the cell the tool has its cursor on, so a shell +// prompt or an editor's caret is visible instead of the screen looking frozen. +// Hidden once the tool has exited: a cursor on a dead screen invites typing. +// +// The splice goes through x/ansi rather than a byte offset: an emulator line +// carries SGR runs, and cutting one at a rune index lands inside an escape +// sequence, which the terminal then executes. ansi.Truncate/TruncateLeft cut by +// *visible* column and keep the styling on both sides intact. +func (m Model) withTermCursor(rows []string, w, h int) []string { + if m.termExit != nil || m.termEmu == nil { + return rows + } + + pos := m.termEmu.CursorPosition() + if pos.X < 0 || pos.X >= w || pos.Y < 0 || pos.Y >= h || pos.Y >= len(rows) { + return rows + } + + content := " " + if cell := m.termEmu.CellAt(pos.X, pos.Y); cell != nil && cell.Content != "" { + content = cell.Content + } + + line := rows[pos.Y] + rows[pos.Y] = ansi.Truncate(line, pos.X, "") + + m.sty().TermCursor.Render(content) + + ansi.TruncateLeft(line, pos.X+1, "") + return rows +} + +// termExitRow is the reserved bottom row: blank while the tool runs, the +// verdict plus the way out once it has finished. +// +// No status message accompanies it — the screen the tool left behind is the +// answer, and this row is the caption on it. +func (m Model) termExitRow(w int) string { + if m.termExit == nil { + return "" + } + s := m.sty() + sep := s.Dim.Render(footerSep) + + verdict := s.Ok.Render("✓ exited") + switch { + case m.termExit.startFailed: + verdict = s.Danger.Render("✕ failed to start") + case m.termExit.killed: + verdict = s.Danger.Render("✕ killed") + case m.termExit.err != nil: + verdict = s.Danger.Render("✕ exit " + termExitCode(m.termExit.err)) + } + + cells := []string{verdict} + // A start failure has no elapsed worth printing and a reason that is the + // whole point, so it takes the middle cell the others spend on duration. + if m.termExit.startFailed { + if reason := m.termExit.err; reason != nil { + cells = append(cells, s.Dim.Render(reason.Error())) + } + } else if el := formatElapsed(m.termExit.elapsed); el != "" { + cells = append(cells, s.Dim.Render(el)) + } + cells = append(cells, m.hint("esc", "close")) + + return fitCells(cells, sep, w, 0) +} + +// termExitCode names the code an exited tool reported, falling back to "?" for +// an error that is not an exit status at all (a wait that failed on its own). +func termExitCode(err error) string { + var ee *exec.ExitError + if errors.As(err, &ee) { + return strconv.Itoa(ee.ExitCode()) } - return m.sty().OverlayBorder.Render(body) + return "?" } // termRunning reports that the overlay has a tool that has not exited yet — diff --git a/internal/model/overlay_term_test.go b/internal/model/overlay_term_test.go index 58cca8b..1527df6 100644 --- a/internal/model/overlay_term_test.go +++ b/internal/model/overlay_term_test.go @@ -2,14 +2,19 @@ package model import ( "errors" + "fmt" + "os/exec" "strings" "sync" "testing" "time" tea "github.com/charmbracelet/bubbletea" + "github.com/charmbracelet/lipgloss" + "github.com/stanlyzoolo/keepkit/internal/loader" "github.com/stanlyzoolo/keepkit/internal/term" + "github.com/stanlyzoolo/keepkit/internal/ui" ) // fakeTermSession drives the overlay handlers without a pty: the test feeds @@ -80,6 +85,12 @@ func (f *fakeTermSession) input(t *testing.T, want string) string { } } +func (f *fakeTermSession) resizes() [][2]int { + f.mu.Lock() + defer f.mu.Unlock() + return append([][2]int(nil), f.resized...) +} + func (f *fakeTermSession) counts() (killed, closed int) { f.mu.Lock() defer f.mu.Unlock() @@ -565,4 +576,374 @@ func TestToolOverlayDoesNotFlushPendingLaunch(t *testing.T) { } } +// laidOutOverlay is a ready model of the given size sitting in the overlay with +// the session adopted and the emulator sized by termGeometry — the state every +// rendering assertion runs against. +func laidOutOverlay(t *testing.T, w, h int, f *fakeTermSession) Model { + t.Helper() + + m := New([]loader.ToolMeta{{Name: "yazi"}}).WithAppVersion("v0.1.0") + mm := mustModel(m.Update(tea.WindowSizeMsg{Width: w, Height: h})) + mm.mode = modeToolOverlay + mm.termToolName = "yazi" + mm.termW, mm.termH, _ = mm.termGeometry() + + nm := mustModel(mm.Update(termStartedMsg{session: f})) + t.Cleanup(func() { nm.closeToolOverlay() }) + return nm +} + +// blockSize reports the overlay frame's size in visible cells. +// +// Measured on the frame itself rather than on the composited View: the layout +// behind it is full of rounded corners of its own (three panels and the status +// bar), so scanning the finished screen for a frame finds the panels' borders +// and reports a size that never changes no matter what the overlay does. +func blockSize(t *testing.T, frame string) (rows, cols int) { + t.Helper() + + lines := strings.Split(stripANSI(frame), "\n") + for _, line := range lines { + if w := len([]rune(line)); w > cols { + cols = w + } + } + if cols == 0 { + t.Fatal("the overlay frame rendered nothing") + } + return len(lines), cols +} + +// The block must not move when the tool exits. Height is held by the reserved +// exit row and width by padding every row to the body width; without either, +// PlaceOverlay re-centres the whole frame at the exact moment the user is +// reading the outcome. +func TestToolOverlayBlockDoesNotMoveAtExit(t *testing.T) { + for _, dim := range [][2]int{{80, 24}, {120, 34}} { + t.Run(fmt.Sprintf("%dx%d", dim[0], dim[1]), func(t *testing.T) { + f := newFakeSession(4) + m := laidOutOverlay(t, dim[0], dim[1], f) + m = mustModel(m.Update(termChunkMsg{session: f, data: []byte("a wide enough line of tool output\r\n")})) + + runRows, runCols := blockSize(t, m.renderToolOverlay()) + + exited := mustModel(m.Update(termExitMsg{elapsed: 12 * time.Second})) + exitRows, exitCols := blockSize(t, exited.renderToolOverlay()) + + if runRows != exitRows { + t.Errorf("block height %d running, %d exited — the reserved row is not doing its job", runRows, exitRows) + } + if runCols != exitCols { + t.Errorf("block width %d running, %d exited — a row is not padded to the body width", runCols, exitCols) + } + }) + } +} + +// Every row inside the frame is exactly the body width, whatever the tool +// printed: a short row leaves a ragged edge and a long one widens the block. +func TestToolOverlayRowsAreExactlyBodyWidth(t *testing.T) { + f := newFakeSession(4) + m := laidOutOverlay(t, 100, 30, f) + // a line carrying SGR, and one longer than the body can hold + m = mustModel(m.Update(termChunkMsg{session: f, + data: []byte("\x1b[1;31mred\x1b[0m short\r\n" + strings.Repeat("x", 200))})) + + rows := m.termBodyRows(m.termW, m.termH) + for i, row := range rows { + if got := lipgloss.Width(row); got != m.termW { + t.Errorf("body row %d is %d cells, want %d", i, got, m.termW) + } + } + + frame := m.renderToolOverlay() + widths := map[int]bool{} + for _, line := range strings.Split(stripANSI(frame), "\n") { + widths[len([]rune(line))] = true + } + if len(widths) != 1 { + t.Errorf("frame lines have %d distinct widths %v, want all equal", len(widths), widths) + } +} + +// A tool whose name is wider than the body must not widen the block. lipgloss +// sizes a border to its widest line, so an unclamped title row is the one thing +// that can push the frame past the geometry everything else agreed on — and it +// would push it past the screen edge, where PlaceOverlay clips silently. +func TestToolOverlayLongToolNameDoesNotWidenTheBlock(t *testing.T) { + f := newFakeSession(4) + m := laidOutOverlay(t, 100, 30, f) + want := frameWidth(t, m.renderToolOverlay()) + + m.termToolName = strings.Repeat("very-long-tool-name-", 8) + if got := frameWidth(t, m.renderToolOverlay()); got != want { + t.Errorf("frame is %d cells wide with a long tool name, want %d", got, want) + } + + // and the same once it has exited, where the title row loses its right cell + m.termExit = &termExitMsg{elapsed: time.Second} + if got := frameWidth(t, m.renderToolOverlay()); got != want { + t.Errorf("exited frame is %d cells wide with a long tool name, want %d", got, want) + } +} + +// frameWidth is the visible width of a rendered frame's widest line. +func frameWidth(t *testing.T, frame string) int { + t.Helper() + + widest := 0 + for _, line := range strings.Split(stripANSI(frame), "\n") { + if w := len([]rune(line)); w > widest { + widest = w + } + } + return widest +} + +// The title names the tool and, while it runs, carries the one key that is +// still keepkit's. Both go once the tool has exited: by then esc is the only +// key that does anything, and the exit row is what says so. +func TestToolOverlayTitleAndKillHint(t *testing.T) { + f := newFakeSession(4) + m := laidOutOverlay(t, 100, 30, f) + + running := stripANSI(m.renderToolOverlay()) + if !strings.Contains(running, "yazi") { + t.Error("the frame does not name the tool") + } + if !strings.Contains(running, `ctrl+\ kill`) { + t.Error("the frame does not advertise the kill chord while the tool runs") + } + + exited := stripANSI(mustModel(m.Update(termExitMsg{elapsed: time.Second})).renderToolOverlay()) + if strings.Contains(exited, `ctrl+\ kill`) { + t.Error("the kill chord is still advertised after the tool exited") + } +} + +// Before termStartedMsg the overlay is already on screen and has to say +// something rather than showing a blank rectangle. +func TestToolOverlayStartingBody(t *testing.T) { + m := New([]loader.ToolMeta{{Name: "yazi"}}) + mm := mustModel(m.Update(tea.WindowSizeMsg{Width: 100, Height: 30})) + mm.mode = modeToolOverlay + mm.termToolName = "yazi" + + if got := stripANSI(mm.renderToolOverlay()); !strings.Contains(got, "starting yazi…") { + t.Errorf("pre-start frame does not say what it is waiting for:\n%s", got) + } +} + +// The child's cursor is a reverse-video cell while it runs, so a shell prompt +// does not look frozen; it goes once the tool has exited, because a cursor on a +// dead screen invites typing. +func TestToolOverlayCursor(t *testing.T) { + // Reverse video is styling, and the default test profile strips it — the + // assertion would pass against a cursor that was never drawn. + forceColor(t) + f := newFakeSession(4) + m := laidOutOverlay(t, 100, 30, f) + m = mustModel(m.Update(termChunkMsg{session: f, data: []byte("$ ")})) + + pos := m.termEmu.CursorPosition() + if pos.X != 2 || pos.Y != 0 { + t.Fatalf("emulator cursor at (%d,%d), want (2,0) after %q", pos.X, pos.Y, "$ ") + } + + rows := m.termBodyRows(m.termW, m.termH) + if !strings.Contains(rows[0], "\x1b[7m") { + t.Errorf("cursor row carries no reverse-video sequence: %q", rows[0]) + } + if got := lipgloss.Width(rows[0]); got != m.termW { + t.Errorf("the cursor splice changed the row width to %d, want %d", got, m.termW) + } + + exited := mustModel(m.Update(termExitMsg{elapsed: time.Second})) + if strings.Contains(exited.termBodyRows(exited.termW, exited.termH)[0], "\x1b[7m") { + t.Error("the cursor is still drawn after the tool exited") + } +} + +// The four outcomes each get their own line, and each names what happened plus +// the way out. No status message accompanies them — the screen is the answer. +func TestToolOverlayExitRow(t *testing.T) { + tests := []struct { + name string + exit termExitMsg + want []string + }{ + {"clean", termExitMsg{elapsed: 12 * time.Second}, []string{"✓ exited", "12s", "esc close"}}, + {"non-zero", termExitMsg{err: exitErrorWithCode(t, 3), elapsed: 4 * time.Second}, + []string{"✕ exit 3", "4s", "esc close"}}, + {"killed", termExitMsg{err: errors.New("signal: killed"), elapsed: 90 * time.Second, killed: true}, + []string{"✕ killed", "1m30s", "esc close"}}, + {"failed to start", termExitMsg{err: errors.New("no such file"), startFailed: true}, + []string{"✕ failed to start", "no such file", "esc close"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := newFakeSession(4) + m := laidOutOverlay(t, 120, 34, f) + m = mustModel(m.Update(tt.exit)) + + row := stripANSI(m.termExitRow(m.termW)) + for _, want := range tt.want { + if !strings.Contains(row, want) { + t.Errorf("exit row %q does not contain %q", row, want) + } + } + if m.statusMsg != "" { + t.Errorf("statusMsg = %q, want the exit row to be the only report", m.statusMsg) + } + }) + } +} + +// While the tool runs the row is reserved and blank: it exists so the block's +// height never changes, not to say anything yet. +func TestToolOverlayExitRowBlankWhileRunning(t *testing.T) { + f := newFakeSession(4) + m := laidOutOverlay(t, 100, 30, f) + + if got := m.termExitRow(m.termW); got != "" { + t.Errorf("exit row = %q while the tool runs, want it reserved and blank", got) + } +} + +// The status bar has a branch of its own, because the global one advertises six +// keys the running tool has taken over — including a q quit that cannot fire. +func TestToolOverlayStatusBar(t *testing.T) { + f := newFakeSession(4) + m := laidOutOverlay(t, 100, 30, f) + + running := stripANSI(m.renderStatusBar()) + if !strings.Contains(running, "yazi running") || !strings.Contains(running, `ctrl+\ kill`) { + t.Errorf("running status bar = %q", running) + } + for _, dead := range []string{"q quit", "t track", "a api"} { + if strings.Contains(running, dead) { + t.Errorf("status bar still advertises %q, which the tool has taken over", dead) + } + } + + exited := stripANSI(mustModel(m.Update(termExitMsg{elapsed: time.Second})).renderStatusBar()) + if !strings.Contains(exited, "yazi exited") || !strings.Contains(exited, "esc close") { + t.Errorf("exited status bar = %q", exited) + } + if strings.Count(exited, "\n") != 2 { + t.Errorf("status bar wrapped: %q", exited) + } +} + +// Geometry: 70% of the screen, minus the frame's own chrome, refusing below the +// minimums. The 80x24 baseline must succeed — it is the default terminal. +func TestTermGeometry(t *testing.T) { + tests := []struct { + w, h int + wantW, wantH int + wantOK bool + }{ + // 80x24: background is 22 rows, 70% -> 56x15, body 52x11 + {80, 24, 52, 11, true}, + {120, 34, 80, 18, true}, + {60, 20, termMinBodyW, termMinBodyH, false}, // too short: body would be 8 rows + {40, 40, termMinBodyW, 22, false}, // too narrow: body would be 24 cols, height is fine + } + + for _, tt := range tests { + t.Run(fmt.Sprintf("%dx%d", tt.w, tt.h), func(t *testing.T) { + w, h, ok := termGeometryFor(tt.w, tt.h) + if ok != tt.wantOK { + t.Errorf("ok = %v, want %v", ok, tt.wantOK) + } + if w != tt.wantW || h != tt.wantH { + t.Errorf("body = %dx%d, want %dx%d", w, h, tt.wantW, tt.wantH) + } + if w < termMinBodyW || h < termMinBodyH { + t.Errorf("body %dx%d is under the floor — a shrunk terminal must clamp, not vanish", w, h) + } + }) + } +} + +// Before the first WindowSizeMsg there is no layout: refused explicitly rather +// than by the percentages agreeing on the floor by coincidence. +func TestTermGeometryNotReady(t *testing.T) { + m := New([]loader.ToolMeta{{Name: "yazi"}}) + m.width, m.height = 200, 60 // set, but no WindowSizeMsg has been applied + + if _, _, ok := m.termGeometry(); ok { + t.Error("termGeometry() reported ok before the first relayout") + } +} + +// A resize follows through to both halves: the emulator is re-laid-out and the +// tool is told, so its redraw lands on a screen of the right shape. +func TestToolOverlayResize(t *testing.T) { + f := newFakeSession(4) + m := laidOutOverlay(t, 120, 34, f) + beforeW, beforeH := m.termW, m.termH + + nm := mustModel(m.Update(tea.WindowSizeMsg{Width: 160, Height: 44})) + + if nm.termW == beforeW && nm.termH == beforeH { + t.Fatalf("geometry unchanged after a resize: %dx%d", nm.termW, nm.termH) + } + if nm.termEmu.Width() != nm.termW || nm.termEmu.Height() != nm.termH { + t.Errorf("emulator is %dx%d, want %dx%d", nm.termEmu.Width(), nm.termEmu.Height(), nm.termW, nm.termH) + } + got := f.resizes() + if len(got) == 0 { + t.Fatal("the tool was never told about the resize") + } + if last := got[len(got)-1]; last[0] != nm.termW || last[1] != nm.termH { + t.Errorf("tool told %dx%d, want %dx%d", last[0], last[1], nm.termW, nm.termH) + } +} + +// Shrinking past the minimum clamps to the floor and keeps the tool running: +// resizing somebody's editor into nothing is bad, killing it outright is worse. +func TestToolOverlayResizeBelowMinimumClamps(t *testing.T) { + f := newFakeSession(4) + m := laidOutOverlay(t, 120, 34, f) + + nm := mustModel(m.Update(tea.WindowSizeMsg{Width: 50, Height: 16})) + + if nm.termW != termMinBodyW || nm.termH != termMinBodyH { + t.Errorf("geometry = %dx%d, want the %dx%d floor", nm.termW, nm.termH, termMinBodyW, termMinBodyH) + } + if nm.mode != modeToolOverlay || nm.termSession == nil { + t.Error("a shrink tore the session down") + } +} + +// The background is dimmed like every other overlay, so the tool's screen is +// the only full-colour thing on it. +func TestToolOverlayDimsTheBackground(t *testing.T) { + forceColor(t) + f := newFakeSession(4) + m := laidOutOverlay(t, 120, 34, f) + + view := m.View() + dim := lipgloss.NewStyle().Foreground(ui.Default.Dim).Render("") + if seq, _, _ := strings.Cut(dim, "m"); !strings.Contains(view, seq) { + t.Error("the layout behind the overlay is not dimmed") + } +} + +// exitErrorWithCode produces a real *exec.ExitError carrying code, so the exit +// row is exercised against the shape a session actually reports rather than a +// zero value whose ExitCode() is -1. +func exitErrorWithCode(t *testing.T, code int) error { + t.Helper() + + err := exec.Command("sh", "-c", fmt.Sprintf("exit %d", code)).Run() + var ee *exec.ExitError + if !errors.As(err, &ee) { + t.Fatalf("sh -c 'exit %d' returned %v, want an *exec.ExitError", code, err) + } + return ee +} + var _ tea.Model = Model{} diff --git a/internal/model/render.go b/internal/model/render.go index 1c04e66..33993aa 100644 --- a/internal/model/render.go +++ b/internal/model/render.go @@ -129,6 +129,16 @@ func (m Model) renderStatusBar() string { if m.mode == modeHotkeys { return style.Render(m.hint("esc", "close")) } + // The overlay's siblings all have a branch here, and without one the bar + // would go on advertising six global keys the running tool has taken over — + // including a q quit that cannot fire. Two states, because the keyboard + // belongs to different owners in each. + if m.mode == modeToolOverlay { + if m.termExit == nil { + return style.Render(s.Dim.Render(m.termToolName+" running") + " " + m.hint(`ctrl+\`, "kill")) + } + return style.Render(s.Dim.Render(m.termToolName+" exited") + " " + m.hint("esc", "close")) + } if m.statusMsg != "" { return style.Render(s.AccentBold.Render(m.statusMsg)) } diff --git a/internal/ui/styles.go b/internal/ui/styles.go index b5d3de5..1d824e9 100644 --- a/internal/ui/styles.go +++ b/internal/ui/styles.go @@ -75,6 +75,14 @@ type Styles struct { OverlayBorder lipgloss.Style OverlayDim lipgloss.Style + // TermCursor marks where the tool running in the [enter] overlay has its + // cursor. It is the one style here that names no theme color, and that is + // the point: reverse video swaps whatever the tool already painted into + // that cell, so the block stays visible against the tool's own palette + // instead of keepkit's — which is the only way one style can mark a cursor + // on a screen keepkit does not control. + TermCursor lipgloss.Style + // The API-usage gauge. GaugeTrack is Theme.SignalDim's only consumer: fill // and track have to read as one bar, which no other pair of roles can do. GaugeFill lipgloss.Style @@ -130,6 +138,7 @@ func NewStyles(t Theme) *Styles { BorderForeground(t.Accent). Padding(0, 1), OverlayDim: fg(t.Dim), + TermCursor: lipgloss.NewStyle().Reverse(true), GaugeFill: fg(t.Signal), GaugeTrack: fg(t.SignalDim), From 79cf6ab0b8d5c13e40789a8cdfbde609cee1e233 Mon Sep 17 00:00:00 2001 From: stanlyzoolo <51911715+stanlyzoolo@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:52:13 +0300 Subject: [PATCH 06/13] feat: run tools in the overlay and delete the tab launcher enter on the run prompt now opens the embedded terminal instead of scripting somebody else's terminal into opening a tab. The refusal comes first and writes no lastRun: a screen with no room for an overlay means the tool never started, and a launch that never happened is not something to remember for the next prompt. internal/launcher goes wholesale, and with it the machinery that existed only to survive an adapter failing - startLaunchCmd, execToolCmd, the launchDone/execDone messages, the one-launch-at-a-time guard, the deferred exec fallback and the flushPendingLaunch wrapper every modal return had to funnel through. setStickyStatus goes too: its only callers were the two launch statuses, and nothing left needs a bar message that outlives its own timer. shellCommand survives and is now what builds argv for the pty, which is what keeps internal/term free of any goos knowledge. Four comments pointing at deleted symbols were re-anchored rather than left dangling: the planFor idiom moves to configdir.baseFor, the two launchTimeout var-seam references to updateTimeout, and acceptsUpdateDetect's mode-gate mirror to the overlay's own reason. Co-Authored-By: Claude Opus 5 (1M context) --- docs/plans/20260814-tool-overlay-terminal.md | 18 +- internal/launcher/launcher.go | 103 ---- internal/launcher/launcher_test.go | 191 ------- internal/model/commands.go | 104 +--- internal/model/launch_test.go | 558 ++++--------------- internal/model/mode.go | 101 +--- internal/model/mode_test.go | 18 +- internal/model/model.go | 128 +---- internal/model/overlay_term_test.go | 17 +- internal/model/status_test.go | 42 +- internal/updater/updater.go | 2 +- 11 files changed, 187 insertions(+), 1095 deletions(-) delete mode 100644 internal/launcher/launcher.go delete mode 100644 internal/launcher/launcher_test.go diff --git a/docs/plans/20260814-tool-overlay-terminal.md b/docs/plans/20260814-tool-overlay-terminal.md index 28a2bfa..e962073 100644 --- a/docs/plans/20260814-tool-overlay-terminal.md +++ b/docs/plans/20260814-tool-overlay-terminal.md @@ -177,13 +177,17 @@ - Modify: `internal/model/mode_test.go`, `internal/model/status_test.go` - Modify/Delete: `internal/model/launch_test.go` -- [ ] rewire `updateRunInput`'s enter: **refusal first** (`termGeometry` incl. `!m.ready`) → statusMsg (`terminal too small to run a tool`-class wording) with **no `lastRun` write** — a launch that never started is not remembered (today's refusal comment, `mode.go:346`, keeps its meaning); on success: `lastRun` write, `startTermCmd`, `modeToolOverlay`; empty-input-cancels unchanged -- [ ] delete `internal/launcher`; in model: `startLaunchCmd`, `execToolCmd`, `launchDoneMsg`/`execDoneMsg`, `m.launchingFor`, `pendingLaunchName`/`Command` + `flushPendingLaunch` and all its modal-return call sites, `launchTimeout`, `launchFallbackStatus`, `notFoundExit`, the `launching…`/`tab open failed…` wordings -- [ ] delete `setStickyStatus` (its only callers were the two launch statuses) and `TestInFlightStatusSurvivesStaleExpiry`; keep `setStatus`/TTL machinery untouched -- [ ] keep `shellCommand` (now feeds the overlay dispatch; refresh its comment and the cross-reference on `updater.customPlan`) -- [ ] update `mode_test.go`: `TestRunInputEnterStoresLastRun` (post-enter mode → `modeToolOverlay`, `lastRun` still written); audit the other `modeRunInput` tests (`TestRunInputOpensPrefilled`, `TestRunInputEscCancels`, `TestRunInputBlankInputCancels`, `TestRunDuringUpdate`, `TestRunInputKeyGuard`) — prompt-opening ones stay, only dispatch-shape assertions change -- [ ] rewrite `launch_test.go` into overlay-dispatch tests: enter→prompt→overlay opens with prefill variants; empty list no-op; rename still clears `lastRun`; refusal writes no `lastRun`; **no test executes the returned cmd** (it would spawn a real pty) -- [ ] run the full gate + `GOOS=windows go build ./...` - must pass before task 6 +- [x] rewire `updateRunInput`'s enter: **refusal first** (`termGeometry` incl. `!m.ready`) → statusMsg (`terminal too small to run a tool`-class wording) with **no `lastRun` write** — a launch that never started is not remembered (today's refusal comment, `mode.go:346`, keeps its meaning); on success: `lastRun` write, `startTermCmd`, `modeToolOverlay`; empty-input-cancels unchanged +- [x] delete `internal/launcher`; in model: `startLaunchCmd`, `execToolCmd`, `launchDoneMsg`/`execDoneMsg`, `m.launchingFor`, `pendingLaunchName`/`Command` + `flushPendingLaunch` and all its modal-return call sites, `launchTimeout`, `launchFallbackStatus`, `notFoundExit`, the `launching…`/`tab open failed…` wordings + - the untrack handler's "drop a pending fallback for this tool" branch goes with them, and the `tea.KeyMsg` dispatch comment now states what the switch actually does rather than what the wrapper used to. +- [x] delete `setStickyStatus` (its only callers were the two launch statuses) and `TestInFlightStatusSurvivesStaleExpiry`; keep `setStatus`/TTL machinery untouched +- [x] keep `shellCommand` (now feeds the overlay dispatch; refresh its comment and the cross-reference on `updater.customPlan`) +- [x] update `mode_test.go`: `TestRunInputEnterStoresLastRun` (post-enter mode → `modeToolOverlay`, `lastRun` still written); audit the other `modeRunInput` tests (`TestRunInputOpensPrefilled`, `TestRunInputEscCancels`, `TestRunInputBlankInputCancels`, `TestRunDuringUpdate`, `TestRunInputKeyGuard`) — prompt-opening ones stay, only dispatch-shape assertions change + - ⚠️ `TestRunInputEnterStoresLastRun` broke for a second reason the plan did not predict: `newTestModel` sets `width`/`height` **without a `WindowSizeMsg`**, so `m.ready` is false and the new refusal fired. It now goes through a real resize — which is the honest fixture, since the dispatch measures the screen. +- [x] rewrite `launch_test.go` into overlay-dispatch tests: enter→prompt→overlay opens with prefill variants; empty list no-op; rename still clears `lastRun`; refusal writes no `lastRun`; **no test executes the returned cmd** (it would spawn a real pty) + - `TestRenameClearsLastRun` already existed in `mode_test.go` and was left there rather than duplicated. +- [x] run the full gate + `GOOS=windows go build ./...` - must pass before task 6 + - ➕ four comments in Go source pointed at now-deleted symbols and were re-anchored here rather than left for Task 8: `updater.go`'s `launcher.planFor` idiom → `configdir.baseFor`, two `launchTimeout` var-seam references → `updateTimeout`, and `acceptsUpdateDetect`'s `launchDoneMsg` mode-gate mirror → the overlay's own reason. ### Task 6: surfaces — hotkeys overlay sweep diff --git a/internal/launcher/launcher.go b/internal/launcher/launcher.go deleted file mode 100644 index 08d6130..0000000 --- a/internal/launcher/launcher.go +++ /dev/null @@ -1,103 +0,0 @@ -// Package launcher decides how to run a tracked tool in a new terminal tab. -// It sits at the bottom of the import graph like internal/updater: no TUI -// knowledge, a pure core (planFor over an injected env lookup) plus a thin -// os.Getenv-facing wrapper (Detect). -package launcher - -import ( - "fmt" - "os" - "strings" -) - -// Plan describes how to open the user's command in a new terminal tab. -// When no supported terminal is detected, Fallback is true and Argv is empty — -// the caller runs the command in the current window via tea.ExecProcess. -type Plan struct { - Argv []string // adapter command, executed directly (not through a shell) - Fallback bool // no scripting API available; run in the current window - Terminal string // human-readable adapter name ("tmux", "iTerm2", …) -} - -// planFor is the pure detection core. The priority chain is deliberate: -// $TMUX first, because inside tmux TERM_PROGRAM names the *outer* terminal and -// a tmux window is the correct "tab" there; then TERM_PROGRAM/KITTY_WINDOW_ID -// checks; anything else falls back. -// -// The user command always executes as `sh -c ` (tmux runs the string via -// the user's shell itself). For tmux/kitty/wezterm the command and tool name -// travel as argv elements — no escaping. For the two AppleScript paths the -// command is interpolated into the script source, with appleScriptQuote as the -// single escaping point. -func planFor(env func(string) string, command, toolName string) Plan { - switch { - case env("TMUX") != "": - // "--" terminates option parsing: a user command edited to start with - // "-" must reach tmux as the shell command, not be eaten as a flag. - return Plan{ - Terminal: "tmux", - Argv: []string{"tmux", "new-window", "-n", toolName, "--", command}, - } - case env("TERM_PROGRAM") == "iTerm.app": - script := fmt.Sprintf(`tell application "iTerm2" - tell current window - set newTab to (create tab with default profile) - tell current session of newTab - set name to "%s" - write text "%s" - end tell - end tell -end tell`, appleScriptQuote(toolName), appleScriptQuote(command)) - return Plan{ - Terminal: "iTerm2", - Argv: []string{"osascript", "-e", script}, - } - case env("TERM_PROGRAM") == "Apple_Terminal": - // Terminal.app opens a *window*, not a tab: tabs are not scriptable - // without System Events. Honest degradation, documented in the plan. - script := fmt.Sprintf(`tell application "Terminal" to do script "%s"`, appleScriptQuote(command)) - return Plan{ - Terminal: "Terminal.app", - Argv: []string{"osascript", "-e", script}, - } - case env("KITTY_WINDOW_ID") != "": - // kitten @ needs a remote-control socket (`listen_on` in kitty.conf → - // exported KITTY_LISTEN_ON, which the subprocess inherits): the adapter - // runs detached with no controlling terminal, so the tty transport of - // plain `allow_remote_control yes` cannot work. Without the socket the - // run fails and the caller's auto-fallback launches in the current - // window instead. - return Plan{ - Terminal: "kitty", - Argv: []string{"kitten", "@", "launch", "--type=tab", "--tab-title", toolName, "sh", "-c", command}, - } - case env("TERM_PROGRAM") == "WezTerm": - // Tab title left to wezterm defaults; naming needs a second pane-id - // round-trip — deliberately skipped. - return Plan{ - Terminal: "WezTerm", - Argv: []string{"wezterm", "cli", "spawn", "--", "sh", "-c", command}, - } - default: - return Plan{Fallback: true} - } -} - -// appleScriptQuote escapes a string for interpolation inside a double-quoted -// AppleScript string literal: backslashes first, then double quotes, then the -// control characters AppleScript literals cannot carry raw (a pasted newline -// would otherwise split the literal and make osascript fail on user data — -// AppleScript understands \n/\r/\t escapes). -func appleScriptQuote(s string) string { - s = strings.ReplaceAll(s, `\`, `\\`) - s = strings.ReplaceAll(s, `"`, `\"`) - s = strings.ReplaceAll(s, "\n", `\n`) - s = strings.ReplaceAll(s, "\r", `\r`) - return strings.ReplaceAll(s, "\t", `\t`) -} - -// Detect resolves the launch Plan for the current environment. Env-only — no -// subprocesses — so it is safe to call inside Bubble Tea's Update. -func Detect(command, toolName string) Plan { - return planFor(os.Getenv, command, toolName) -} diff --git a/internal/launcher/launcher_test.go b/internal/launcher/launcher_test.go deleted file mode 100644 index 58182a8..0000000 --- a/internal/launcher/launcher_test.go +++ /dev/null @@ -1,191 +0,0 @@ -package launcher - -import ( - "reflect" - "strings" - "testing" -) - -// envFrom builds an env lookup over a fixed map; missing keys return "". -func envFrom(m map[string]string) func(string) string { - return func(k string) string { return m[k] } -} - -func TestPlanFor(t *testing.T) { - tests := []struct { - name string - env map[string]string - command string - toolName string - wantTerminal string - wantFallback bool - wantArgv []string - }{ - { - name: "tmux", - env: map[string]string{"TMUX": "/tmp/tmux-501/default,1234,0"}, - command: "yazi", - toolName: "yazi", - wantTerminal: "tmux", - wantArgv: []string{"tmux", "new-window", "-n", "yazi", "--", "yazi"}, - }, - { - name: "tmux wins over TERM_PROGRAM", - env: map[string]string{ - "TMUX": "/tmp/tmux-501/default,1234,0", - "TERM_PROGRAM": "iTerm.app", - }, - command: "fzf", - toolName: "fzf", - wantTerminal: "tmux", - wantArgv: []string{"tmux", "new-window", "-n", "fzf", "--", "fzf"}, - }, - { - name: "kitty", - env: map[string]string{"KITTY_WINDOW_ID": "3"}, - command: "dive nginx:latest", - toolName: "dive", - wantTerminal: "kitty", - wantArgv: []string{"kitten", "@", "launch", "--type=tab", "--tab-title", "dive", "sh", "-c", "dive nginx:latest"}, - }, - { - name: "wezterm", - env: map[string]string{"TERM_PROGRAM": "WezTerm"}, - command: "btop", - toolName: "btop", - wantTerminal: "WezTerm", - wantArgv: []string{"wezterm", "cli", "spawn", "--", "sh", "-c", "btop"}, - }, - { - name: "empty env falls back", - env: map[string]string{}, - command: "yazi", - toolName: "yazi", - wantFallback: true, - }, - { - name: "unknown TERM_PROGRAM falls back", - env: map[string]string{"TERM_PROGRAM": "ghostty"}, - command: "yazi", - toolName: "yazi", - wantFallback: true, - }, - { - name: "tool name with spaces stays one argv element (tmux)", - env: map[string]string{"TMUX": "x"}, - command: "docker run -it alpine", - toolName: "my tool", - wantTerminal: "tmux", - wantArgv: []string{"tmux", "new-window", "-n", "my tool", "--", "docker run -it alpine"}, - }, - { - name: "dash-leading command survives tmux option parsing", - env: map[string]string{"TMUX": "x"}, - command: "-la", - toolName: "ls", - wantTerminal: "tmux", - wantArgv: []string{"tmux", "new-window", "-n", "ls", "--", "-la"}, - }, - { - name: "unicode tool name stays intact (kitty)", - env: map[string]string{"KITTY_WINDOW_ID": "1"}, - command: "ls", - toolName: "инструмент", - wantTerminal: "kitty", - wantArgv: []string{"kitten", "@", "launch", "--type=tab", "--tab-title", "инструмент", "sh", "-c", "ls"}, - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - got := planFor(envFrom(tc.env), tc.command, tc.toolName) - if got.Fallback != tc.wantFallback { - t.Fatalf("Fallback = %v, want %v", got.Fallback, tc.wantFallback) - } - if got.Terminal != tc.wantTerminal { - t.Errorf("Terminal = %q, want %q", got.Terminal, tc.wantTerminal) - } - if tc.wantFallback { - if len(got.Argv) != 0 { - t.Errorf("fallback plan carries Argv %v, want empty", got.Argv) - } - return - } - if !reflect.DeepEqual(got.Argv, tc.wantArgv) { - t.Errorf("Argv = %#v, want %#v", got.Argv, tc.wantArgv) - } - }) - } -} - -func TestPlanForITerm(t *testing.T) { - got := planFor(envFrom(map[string]string{"TERM_PROGRAM": "iTerm.app"}), `echo "hi"`, "echo tool") - if got.Terminal != "iTerm2" || got.Fallback { - t.Fatalf("plan = %+v, want iTerm2 non-fallback", got) - } - if len(got.Argv) != 3 || got.Argv[0] != "osascript" || got.Argv[1] != "-e" { - t.Fatalf("Argv = %#v, want [osascript -e