feat: run tools in an overlay terminal inside keepkit - #68
Closed
stanlyzoolo wants to merge 13 commits into
Closed
Conversation
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 <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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;<mod><final> - 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Measured after the change: 64x20 in all five self states against the 76x20 budget, so the longer wording costs nothing that was scarce - it is the height that sits at its ceiling, not the width. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every other overlay test uses a fake session, which is what keeps them fast and deterministic - and also means none of them would notice if waitForTermChunkCmd and term.Session disagreed about their own channel. This one pumps the command chain the way the tea runtime does, from start through the drain to the exit row, against a real printf on a real pty. It still does not execute the run prompt's returned cmd. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The feature gets the fourth deep-design doc: the Update-only emulator rule and why input still needs a goroutine, the teardown that must not call Emulator.Close, esc belonging to the tool, the two invariants that keep the block from moving, and what was deleted along with the reason most of it existed. docs/research/pty-stack.md keeps the stack's own rationale - what was rejected, what each pin buys, and the two upstream issues we live with. CLAUDE.md, ARCHITECTURE.md and README.md follow: internal/term replaces internal/launcher, modeToolOverlay joins the enum, and every comment anchored to a deleted symbol moves to a surviving one. The docs-sync sweep caught two drifts the plan had not listed: the mermaid graph was missing term --> proc, and README's Stack omitted ultraviolet, which key translation made a direct dependency. CLAUDE.md shrinks by 1896 characters - the Run bullet became an invariant summary plus a link, which more than paid for the new rows. It is still over the 140k warning threshold it was already over before this work; splitting another section is left as its own decision. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every internal/term test timed out on the linux runner and the model package hung on TestToolOverlayEndToEndWithRealPty until go test's 600s axe fell - while the same suites pass on macOS. xpty keeps both halves of the pty pair open in the parent and its Start never closes the slave (creack/pty's own StartWithAttrs does, with a defer right after Open - xpty rewired the same fds without that line). On Linux a read on the master returns EIO only once every slave fd is closed, the parent's copy included, so the child's exit never ended stream's read, the Exit event never fired, and every consumer of Events() waited forever. macOS revokes the terminal when the session leader exits and answers EOF regardless, which is why the leak was invisible locally. closeSlave drops the parent's copy right after pty.Start succeeds, mirroring the attr_unix/attr_windows split (ConPTY is one handle, not a pair - nothing to close there). Session.Close now filters os.ErrClosed: xpty's Close closes master then slave, and the slave's second close is the expected shape of a clean teardown, not a failure to report. Verified in a golang:1.25 container: the 9 session tests went from 10s timeouts to 0.5s, the e2e from a 600s package hang to 0.013s; the full -race suite stays green on darwin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three hardenings in the overlay's message flow, each pinned by a test. A WindowSizeMsg landing between the run-prompt enter and termStartedMsg moved termW/termH with no session to follow them: the emulator was then built at the new size while the pty kept the keypress-time winsize, and resizeToolOverlay's nothing-changed early return made the divergence permanent. Adoption now resizes the session unconditionally - in the common no-resize case that is a same-size TIOCSWINSZ, which the kernel drops without a SIGWINCH, so the child never notices. The stale branch of handleTermStarted killed and closed an unadopted session but never drained its events, converting its own scenario into a reader goroutine parked forever on a full event buffer with a never- reaped child behind it. It now returns a bounded drain cmd, and its doc comment says what is true: the branch is unreachable as wired and kept for fakes and future mode writers. A paste arrived as typed runes: vim auto-indented every pasted line. KeyRunes with Paste set now routes through Emulator.Paste, which brackets the block with the ?2004 markers exactly when the tool asked for them and passes it bare otherwise. The design doc's translation table gains the fourth row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
statusMsgTTL was described as "shrunk by tests the same way as launchTimeout" in CLAUDE.md - and the re-anchoring in the deletion commit moved the two sibling references onto updateTimeout, which is a const and cannot be shrunk, so both statements it created were false on arrival. All three sites now stand on shrinkStatusTTL itself: statusMsgTTL is the last shrinkable timeout seam. The "no unwrapped cmd left" absolute in CLAUDE.md and ARCHITECTURE.md died the day it was written: handleTermChunk re-emits the exit as a bare closure. Both now name it - a prebuilt value that cannot panic, the same safe-by-construction category execToolCmd occupied. Also drops the double blank line the deleted pendingLaunch fields left in model.go, the one gofmt finding this branch introduced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TestCleanReadmeMarkdownPathologicalInputIsFast/leading_bullets failed three CI runs in a row at 2.003-2.031s against its 2s budget - a fair pass, not a regression: this branch's path times identically to main's on the same hardware (~1.05s vs ~1.01s under -race locally), the race detector alone costs the pass ~30x (~30ms uninstrumented), and CI's shared two-core runners double that again, landing exactly on the line. A raceEnabled const (build-tagged _test pair, so nothing ships) triples the budget under -race. The guard is against the 8.4s rcLineContent freeze class, so 6s under race still catches the regression while no longer failing on runner weather. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
enterin[1] toolsnow runs the typed command on a pseudo-terminal inside keepkit — a bordered block over the dimmed layout with the whole keyboard handed to the tool. One path covers everything:vimdraws and edits in it,fzffilters,rg --versionprints two lines and the block stays untilesc.This replaces the tab launcher that scripted someone else's terminal into opening a tab.
internal/launcher(tmux, iTerm2, Terminal.app, kitty, WezTerm adapters) is deleted wholesale, along with the machinery that existed only to survive an adapter failing:startLaunchCmd,execToolCmd, the launch/exec done messages, the one-launch guard, the deferred exec fallback,flushPendingLaunchandsetStickyStatus.What's new:
internal/term— pty session (start, resize, kill-group teardown), goos-free: argv still comes fromshellCommandinternal/model/overlay_term.go—modeToolOverlay: thevt.Emulator, keyboard routing, frame/cursor/outcome renderingUpdatetouches the emulator's screen state (x/vt's buffer is unsynchronised, vt: data race on Emulator.closed between Read and Close charmbracelet/x#879); the input goroutine is the single documented exceptiondocs/design/tool-overlay.md(fourth deep-design doc) +docs/research/pty-stack.md; CLAUDE.md, ARCHITECTURE.md and README synced — the sweep also caught the missingterm --> procmermaid edge and ultraviolet missing from the Stack listPreflight green: build, vet,
test -race, lint (0 issues).