feat(tui): fill the prompt block and drop the cover after the first turn - #361
Conversation
Two presentation defects in the chat transcript, kept together because both change what the virtualized transcript has to reserve rows for. The cover was never a screen. It is transcript row 0, a message with kind 'intro', and capHistory pinned it at index 0 so it was exempt even from the history cap. It renders 35 terminal rows, so the first answer was appended below a full screen of wordmark and session panel instead of replacing it. hideIntroAfterFirstTurn drops it from the view once a user or assistant row exists. The row stays in historyItems, because the late session.info event patches itself onto it and /export and the slash handlers read the session's own list rather than whatever is on screen. Startup notices and slash output are not a turn, so a host that warns about its credentials on boot still gets its cover. The rows and the layout's copy both come from the filtered array. appLayout compares row.index against indices it derives from transcript.historyItems, so filtering one and not the other would move the inter-turn separator and the todo panel one row off. The person's own message now renders on a filled background, so a turn reads as an inserted card rather than one more line of prose. userBg is a curated literal per tier and scheme, the way the rest of the reduced-tier palette is carried, and is skinnable through ui_user_bg. Below 256 colors there is no shade between black and brightBlack, so the fill is skipped and the prompt chevron carries the row alone. The two padding rows are drawn at every tier so estimatedMsgHeight can reserve a row count without reading the terminal's color capability, and it now reserves four rows for a user message rather than two. Verified against the real renderer: ink-testing-library ships its own reconciler and drops both foreground colors and background fills, so the escape codes are only observable through renderSync. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the complete nine-file diff and covered the repository rules (AGENTS.md, CONTEXT-MAP.md, and the TUI glossary), the transcript/rendering callers and relevant history, backward compatibility, and whether the tests weakened existing assertions. I traced the visible/raw history split through useMainApp, appLayout, virtual row heights, sticky prompts, live todos, session create/resume/compress, late session.info, and slash/raw-history consumers. The indices stay aligned while the retained cover row remains available to session-side consumers. I also checked the prompt fill through theme resolution, skin fallback, terminal color tiers, wrapping, and reserved-height accounting.
Validation on this head:
- Focused changed suites: 3 files, 63 tests passed.
- Full TUI suite: 87 files, 1009 tests passed.
npm run type-check --prefix ui-tui: passed.npm run lint --prefix ui-tui: 0 errors, 22 warnings on untouched lines.- Commit-message check and
git diff --check: passed. - Merge tree against latest fetched
main(0d2f2c25): clean.
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this revision preserved the reviewed patch and the landed tree is clean.
This was a revision-specific delta review, not a repeat of the earlier feature review. git range-diff reports the feature commit unchanged, and the old/new stable patch IDs are identical. The absorbed main changes have no path overlap with the nine feature files; I rechecked the adjacent virtual-history identity contract, repository-rule impact, callers, backward compatibility, and test assertions, and found no new failure path or weakened test. The b562f332 tree is also identical to the squash-merged main tree at 22b71765.
Validation on the combined tree:
- Focused feature plus virtualization suites: 4 files, 65 tests passed.
- Full TUI suite: 90 files, 1046 tests passed.
npm run type-check --prefix ui-tui: passed.- Current-head GitHub checks: all passed.
#354) ## Summary `ask_user` advertised things it did not do, and a batch cost the user more than one wait. This closes both, and gives the prompt the fields it needs to render a batch honestly. - **Two schema fields nothing read.** `multiple` and `custom` were declared per question and never passed to the broker, so a model asking for a multi-select got a single-select and never learned why. Both are removed. Multi-select is deliberately not implemented here: it would need a new answer syntax on the chat-channel surface, which has no dialog to hold a multi-select, and that is a larger decision than removing a field that never worked. - **One deadline per call, not per question.** A batch was N round-trips with N independent timeouts, so a three-question call could hold a lane for three times the surface's wait. The call now shares one budget, and a spent budget stops the batch instead of opening a fresh wait on every question left. - **The budget is configuration.** `tools.ask_user.timeout` (600s, must be positive) reaches the tool through `AgentLoop`, which is where the config already is. It deliberately does not read config at the transport: the TUI builds its broker before any config is loaded, and reading it there put a schema-validation failure in the path of the RPC server coming up. - **Calls that would waste the user's time are rejected before rendering**, with a message that steers the retry: more than four questions, a duplicate question text, or a question with exactly one option, which is not a decision. A duplicate option label is a typo with one obvious reading, so it is deduped rather than rejected. Zero options remains a free-form question. - **`header` and `recommended`.** A short chip label, and a 0-based index into `options` naming the option the agent would pick. When nobody answers, the text the model reads now names that option; before, the only signal was that no answer arrived. - **A recommendation is resolved against the options as submitted.** The index counts what the caller sent, but the list was deduplicated before the index was resolved against it. With `["a","a","b","c"]` and `recommended: 2` the caller means `"b"` while the broker was handed `"c"`, so the surface marked the wrong row and a timeout told the model the wrong intended fallback; an index past the deduped length (`3` on that list) failed the range check and the recommendation was dropped without a word. The label is now resolved before dedup narrows the list. Dedup keeps every distinct label, so the resolved label is always still one of the choices the surface can mark. - **An undeliverable question is reported, not dropped.** A question for a conversation with no live source was dropped with a log line the broker could not see, so the round-trip waited out its whole budget on a question nobody would ever see. The channel adapter now raises `QuestionUndeliverableError` and the broker fails safe at once. - **The prompt renders the batch.** Position in the call, the questions still to come, the recommended option marked, and the remaining budget counting down. Tab attaches a note to a selection, so an answer no longer has to be either a choice or free text. On a chat channel the position rides in the message text. - **The countdown renders whole seconds.** `timeout_s` is a remaining-time subtraction, so a 90-second budget arrives as `89.99999912502244` and the prompt decrements that same fractional value once a second. The formatter passed it straight through, so the line read `1m 29.99999912502244s` on every tick and widened the prompt that the padding exists to keep steady. The value is now ceiled before the branch is chosen, since ceiling inside the branches renders a ceiled `59.7` as `60s` rather than `1m 00s`. - **The `Other` row answers to its own number.** This one predates the rest of the branch. The options list draws `Other` as one more numbered row, but the quick-pick handler bounded itself at the number of real choices, so the last row was numbered and unreachable -- and worse than inert: the keystrokes that followed were swallowed by the options handler, so the next Enter submitted whichever option was still highlighted. Someone who thought they were typing a free-form answer sent a selection they never made. Its number now opens the text input, which is what Enter on that row already did, and the hint names the range it can take. The wire payload gains `header`, `recommended`, `timeout_s`, `index`, `total` and `batch`. `clarify.request` is a notification and is not part of `rpc-schema/openrpc.json`, so no generated artifact changes; `npm run lint:rpc` confirms `generated.ts` is still in sync. `deep_research` shares the same broker and passes none of the new fields, so its prompt is unchanged except that it now shows the countdown, which was always the real deadline. `timeout_s` stays fractional on the wire deliberately: it is also what the model is told about the budget, and rounding it at the source would change that value to serve a display concern. The formatter is the layer with a width to protect. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other Mostly fixes; the prompt affordances and the config field are additive. ## Verification Every new test was watched failing before the code that makes it pass. Rebased onto `22b71765`, which collapsed the earlier `Merge branch 'main'` commit into a linear six-commit history. The base gained #361 and #362; the only file this branch and those commits both touch is `raven/agent/loop/main.py`, and the replay was clean. `git merge-tree --write-tree` needs git 2.38 and this box has 2.34.1, so the pre-push check was that file-overlap table plus a full re-run at the new head rather than a dry-run merge. Everything below is from the new head, not carried forward. ``` python -m pytest tests/ -q 82 failed, 6576 passed, 47 skipped, 13 deselected # same suite on the new base 22b7176, in a separate worktree: 82 failed, 6545 passed, 47 skipped, 13 deselected # full failing-ID lists from both runs, sorted and diffed: diff branch_fail.txt base_fail.txt -> identical (0 introduced, 0 fixed) ``` The 82 failures are inherited: the base fails the same 82 IDs on this box, and the sets are identical rather than merely the same size. They are `test_tui_rpc_session.py` (58), `test_cli_cron_commands.py` (7), `test_cli_import_commands.py` (5), `test_tui_commands_error_codes.py` (3), `test_cli_onboard_commands.py` (3), and 6 more spread across 5 files. The +31 passing tests are the ones added here. Suites that exercise the changed code directly: ``` python -m pytest tests/test_ask_user_tool.py tests/test_question_broker.py \ tests/test_cli_gateway_commands.py tests/test_config_schema.py -q 81 passed npm run test -- --run src/__tests__/clarifyPrompt.test.tsx 11 passed npm run type-check -> clean npm run lint:rpc -> OK: generated.ts in sync python -m ruff check raven tests -> All checks passed! python -m ruff format --check raven tests -> 849 files already formatted python -m scripts.check_commit_messages github/main..HEAD -> exit 0 scripts/check_large_files.py -> exit 0 ``` Beyond the suites, the tool was exercised over the real path -- a real `ToolRegistry` (so `cast_params` and the schema validator run), a real `QuestionBroker`, and answers delivered through the real `clarify.respond` handler on a real `Dispatcher`: - the three rejections emit **zero** prompts, so a malformed call costs the user nothing, and the steer text reaches the model intact through the registry; - a batch answered by a responder taking 0.25s per question is handed `10.0000s`, `9.7495s`, `9.4991s` -- each question inherits what the last one left; - a batch whose budget is spent emits **one** prompt, not one per question; - an undeliverable question returns in `0.001s` against a 30s budget. `test_registry_dispatch_and_the_real_clarify_respond_route` was added from that exercise, because no committed test drove the registry before. It was proved non-vacuous by renaming the `recommended` keyword the tool passes the broker and watching it go red. The two review fixes were each proved the same way: - The recommendation cases were run against the deduped resolution and failed with `assert 'c' == 'b'` (wrong label) and `assert '' == 'c'` (recommendation dropped), then passed once the resolution moved ahead of dedup. `test_out_of_range_recommended_index_is_ignored` passes unchanged -- it has no duplicates, so it never encoded this. - The countdown cases were run against the pass-through formatter and failed on `expected ... to match /1m 30s/` and `/1m 00s/`, covering both the fractional payload and the `59.7` boundary that ceiling creates. `prompts.tsx` and `clarifyPrompt.test.tsx` are reported by Prettier, and were before this branch touched them: the hunks it wants are at lines this branch does not change (a JSX ternary and a `renderSync` call), so they are left alone rather than reformatted into this diff. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [x] User-facing docs or screenshots are updated when needed `docs/Proactivity-Implementation.md` documented the two removed fields and the old narrow payload, so that paragraph is rewritten. `ui-tui/dist/entry.js` is gitignored, so testing the prompt by hand needs `npm run build --prefix ui-tui` first. ## Risk User-visible changes: - A model sending five questions, a duplicate question, or a one-option question now gets an error instead of prompting the user. The error text tells it what to do instead. - A batch that used to get a fresh timeout per question now shares one. A slow user answering question 3 of 4 past the budget gets the rest recorded as unanswered rather than being asked. - The prompt shows a countdown where it showed none, including for `deep_research`, and that countdown now reads in whole seconds. - A recommendation sent as an index past the deduped option count used to be dropped silently and now resolves. That is what the schema promises, but it is a real change for a caller that had been sending such an index. - Typing the `Other` row's number now opens the text input instead of doing nothing. Rollback is the six commits; nothing is persisted and no format changes on disk. The config field defaults to the previous 600s, so an untouched config behaves as before. Checked and deliberately not fixed, to keep this diff to one subject: - `QuestionBroker.await_question` still returns its default on `asyncio.CancelledError`, which swallows an outer cancellation. It sits beside the handler added here, but changing it means auditing how the agent loop reacts to a cancelled turn. - The gateway's inbound gate calls `pending_req(cid)` and then `reply(...)`. If the question times out between the two, the message answers nothing and starts no turn. `reply` already returns a bool, so using it as the condition would close this. - The registry's schema validator does not implement `maxItems`, so the cap in the schema is advisory to the model and `_prepare` is what enforces it. That is deliberate -- the model gets the steer text rather than a generic validation error -- and a test pins the two statements of the cap together so they cannot drift. - `ApprovalPrompt` in the same file hardcodes `1-2 quick pick`, which is correct for its two fixed options and has no `Other` row, so it is not the same bug. - `approvalRemainingSeconds()` feeds the approval countdown in the same file and is already `Math.max(0, Math.ceil(...))`, so it needs nothing here. It is also where the `ceil` convention comes from: the clarify countdown now rounds the same direction as the approval one rather than inventing a second rule. The two are still not made to share a formatter -- one takes an absolute deadline and the other a remaining duration. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes No new secret, no new surface, and no asset moved onto an unauthenticated one. The new config field is a timeout. ## Related Issues N/A --------- Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
…lipboard path lying (#360) ## Summary A TUI that enables mouse tracking owns the drag, so the terminal never builds a native selection and its own copy shortcut has nothing to copy. Copy-on-select is what makes a transcript selection copyable at all -- but the subscription bailed out on `!isMac`, so on Linux and Windows a drag highlighted text and copied nothing. This lifts that gate, and fixes what was found while making the result observable. **Copy-on-select on every platform.** The subscription moves out of `useMainApp` into `subscribeCopyOnSelect()`. Inlined in a hook that needs a live gateway to start, none of its four guards could be tested; each now has a case, and each was checked by removing only that guard and watching the suite go red. The read of the bus state stops being an unchecked cast: the ambient `useSelection()` declaration types it as `unknown`, so the module narrows it instead. **The copy is reported to the transcript.** Nothing on screen changes when a drag ends, so a line is written once a clipboard path has actually taken the text. The callback fires on a non-empty result only, since `copySelectionNoClear()` resolves to an empty text when nothing reached the clipboard. The first report of a session carries the path caveat, later ones stay terse: OSC 52 is the one path a terminal can still refuse, and the first copy is when a user is looking for the reason a paste came up empty. **The reported path comes from the write, not from the environment.** A path predicted from env state cannot be right in general, because the environment does not record what happened: inside tmux a `load-buffer` that fails falls through to raw OSC 52 and leaves `TMUX` set exactly as the case that worked. With a stale tmux socket the bytes went out as OSC 52 while both callers said "copied to the tmux buffer", pointing the user at a `set-clipboard` setting that was never involved. `setClipboard()` already computed the three facts that decide this -- whether native was attempted, whether the buffer loaded, whether a sequence was emitted -- so it now reports which one took the text, and null when none did. The value travels with the copied text out of `copySelectionNoClear()` and `copySelection()` to the two call sites that display it. The predictor is deleted rather than left exported from the package: after the rewiring it had no callers, and a helper that can name a path the data did not take is a defect waiting for the next caller. Its env-matrix cases were assertions about the predictor itself, so what replaces them drives the real `setClipboard()` with `tmux` stubbed, including the failed-load fallback. Reporting an observed path also has to wait for the observation. On Linux the native tool is discovered by a probe, and `copyNative()` answered the first call before that probe settled -- a display server means a tool could exist, not that one does. Naming the path from that optimism claimed a native write on a machine with `DISPLAY` and no `wl-copy`, `xclip` or `xsel`, and the same call reported failure once the probe finished. The first call now answers with the probe's own result. It is still started before the tmux await, so the probe runs alongside `load-buffer` rather than ahead of the report, and later calls still answer synchronously from the cache. The copy never ran ahead of that probe either -- the tool is spawned inside it -- so only the report did. **The path caveat is scoped to a session, not to the process.** The first copy of a session carries the caveat and later ones stay terse, but the flag holding that lived in a hook that outlives the session: `newSession()` and `resumeById()` replace `ui.sid` without remounting `useMainApp`, so every session after the first opened with the terse line and the user never learned an OSC 52 paste can come up empty. The tally moves into a reporter keyed on a session identifier, so the state and the thing it is scoped to live together and the boundary is unit-testable. The hook reads the sid through `getUiState()`, which keeps a session change from tearing down the bus subscription. **The report must not overclaim.** OSC 52 hands bytes to the terminal and the terminal decides whether to keep them, and nothing in the write path caps or chunks the payload. Measured on the real code: one 200x50 viewport of CJK is a 40 KB escape sequence, and a selection dragged through 2000 rows is 536 KB in a single sequence. Terminals drop an oversized sequence without a word, and `setClipboard()` reports success for all of it because bytes were written to stdout. So that path reports what it sent; native and tmux, which really did write a clipboard, still say copied. The count was UTF-16 code units, which reads three emoji as six characters and a combining accent as two, so it is counted by grapheme now. CJK was already right, being one code unit per character. **The documented env knobs were dead.** The OSC 52 override and the clipboard debug switch were readable only under the upstream `HERMES_TUI_` names, while every knob this project documents uses `RAVEN_TUI_` and `/copy`'s own failure hint names `RAVEN_TUI_FORCE_OSC52` and `RAVEN_TUI_DEBUG_CLIPBOARD`. Following that hint changed nothing. Both spellings are now read, `RAVEN_TUI_` first, with the `HERMES_TUI_` names kept as aliases so an environment that worked before still works. The debug switch moves behind `clipboardDebugEnabled()` so its four call sites cannot drift apart. ## Type - [x] Fix - [x] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other Two boxes: the platform gate is the feature, the clipboard-path and env-name defects are fixes found while making it observable. ## Verification Run from `ui-tui/`, on Linux, node 22, at branch head after the rebase. ``` npx vitest run --no-file-parallelism Test Files 91 passed (91) Tests 1066 passed | 13 skipped (1079) npx tsc --noEmit -p tsconfig.json clean, exit 0 npm run lint 22 problems (0 errors, 22 warnings) python3 -m scripts.check_commit_messages github/main..HEAD exit 0 ``` The suite is run with `--no-file-parallelism` deliberately: the ink render tests fail under default worker parallelism at this suite size, and no CI job runs them. The 22 lint warnings are the baseline, not new: 22 before the branch and 22 after, none of them in a file this branch touches. Every file this branch touches passes `prettier --check`. Two of them did not at first, and that was this branch's doing rather than the base's: dropping a name from the `@hermes/ink` import and adding a wide function signature left both inside the print width while still wrapped. They are formatted in their own commit. The files already unformatted on `github/main`, `chatStream.ts` among them, are left alone rather than reformatted into this diff. Suites that exercise the changed code directly: `src/__tests__/copyOnSelect.test.ts` (12 cases), `src/__tests__/clipboard.test.ts` (30 cases), `packages/hermes-ink/src/ink/termio/osc.test.ts` (39 cases). An environment gap that made the first pass of this verification worthless, since it would silently affect anyone reviewing from a worktree: `ui-tui/node_modules` here was a symlink to another checkout's, so `node_modules/@hermes/ink -> ../../packages/hermes-ink` resolved against the symlink target and landed on that other checkout's copy of the package. `tsc` and the dist build therefore read an unmodified `hermes-ink` while the app code under test was the edited one. Fixed by giving the worktree a real `node_modules` whose `@hermes/ink` points at its own package, confirmed with `tsc --listFiles` naming the edited `ink.tsx`, then rebuilt and re-ran everything. Every number above is from after that. New tests were proved load-bearing rather than assumed: - Each of the four guards in `subscribeCopyOnSelect()` was removed on its own and the suite watched go red (1, 1, 2 and 2 failures), then restored. - The three cases that assert a copy did *not* happen are each paired with a positive assertion on the same subscription, because a bus that was never wired up satisfies the negative half by itself. - The path-reporting cases were checked by reverting the derivation to the deleted predictor's rule and watching the failed-load case report `tmux-buffer` again. - The Linux first-probe case was checked by reverting `copyNative()` to its optimistic `return true` and watching it fail on `success: true, path: 'native'` where it now asserts `success: false, path: null`. Establishing that this one was introduced here rather than inherited took running the same scenario against `github/main`, which reported `path: 'osc52'` for it: `success: true` with nothing written predates the branch, the native claim does not. - Of the three session-scoping cases, one is load-bearing: reverting the reporter to a process-wide flag fails "spends the caveat once per session". The other two -- a session returned to, and two reporters not sharing a tally -- pass with the flag as well, and are there to pin the property against a later refactor. Said plainly rather than implying all three prove the fix. One pre-existing assertion is flipped rather than kept. It pinned the terse copy-on-select line to "copied" on the OSC 52 path, which is the overclaim this branch removes, so the assertion encoded the defect instead of the behaviour worth holding. Rebased onto `22b71765`. The base gained #361, #362 and #355; the only file this branch and those commits both touch is `ui-tui/src/app/useMainApp.ts`, where #361 reworked the cover and prompt block while this branch replaced the copy-on-select effect in the same hook. The replay was clean and the whole verification above was re-run at the new head rather than carried forward. `git merge-tree --write-tree` needs git 2.38 and this box has 2.34.1, so the check was the file-overlap table plus the re-run, not a dry-run merge. Manual check of the rendered artifact, since `raven tui` runs the prebuilt `ui-tui/dist/entry.js` and nothing rebuilds it automatically: `npm run build`, then confirmed the new strings are in `dist/entry.js`. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed No doc change. `CONTEXT.md` has no clipboard or selection entry, and copy-on-select was already the term in use in the vendored fork, so nothing new is coined. The `/help` hotkey table is deliberately untouched: select-then-copy is ordinary terminal behaviour and does not need advertising. ## Risk Behaviour changes for users who are not on macOS: a drag now writes the clipboard and adds a transcript line. On macOS nothing changes except the wording of that line. `/copy` keeps its shape and gains the path the write actually took. One API change inside the vendored fork: `Ink.copySelection()` and `copySelectionNoClear()` return `{ text, path }` instead of the copied string, and `getClipboardPath()` is gone from the package's exports. Internally `copyNative()` may now answer with a promise on Linux's first copy, which `setClipboard()` awaits after starting the tmux write; no caller outside that function sees it. Both are consumed only within this repo, and the ambient declaration, the app interface and the two call sites move with them. Rollback is per concern. Reverting the feature commit restores the platform gate and leaves the fixes, which stand on their own. Reverting all of them returns the files to their base state; no data, config or on-disk format is involved, and no migration exists to undo. Checked and deliberately not fixed: - The OSC 52 write has no size cap and no chunking, so a large selection is handed to the terminal whole and silently dropped past whatever that terminal tolerates. This is why the OSC 52 path reports what it sent rather than claiming a copy. Capping or chunking it means changing the vendored write path and picking a threshold per terminal, neither of which belongs in this branch. - `Ink.copySelectionNoClear()` calls `getSelectedText()` outside its own `try`, so a throw there rejects the promise. The base already left that rejection unhandled at a bare `void` call, and adding a `.then()` does not change it. Fixing it means restructuring a vendored method this branch has no other reason to touch. - The effect's dependency array gains `sys`. It is provably stable (`useCallback` over `appendMessage`, itself `useCallback` with `[]`), so the subscription is not re-created in practice. If it ever were, the version de-dupe would reset and one extra line could be written; the path caveat would not repeat, because that tally now lives in a ref that survives the effect. - Running `/copy` before the first drag of a session shows the path caveat twice, once from each lane. Sharing that state between an explicit command and an automatic one to save a duplicated sentence is not worth the coupling. - Two narrative comments in `copyOnSelect.test.ts` were reduced rather than removed: each kept the constraint it was wrapped around (that the platform case can only mean something on a non-macOS runner, and that an empty result is the whole signal that nothing reached the clipboard) and lost the "defect this closes" framing. Reading section 1.1 as rejecting the task-context wrapper rather than the constraint underneath is a judgement call, flagged as one. The selection bus is faked in the new tests. The real one returns no-ops outside a fullscreen Ink instance bound to a TTY, so there is no way to drive it from vitest; the fake mirrors the ambient `useSelection()` contract, including `getState(): unknown`, which is why the narrowing is exercised rather than assumed. Flagging it because a hand-rolled stand-in for a real type is exactly the blind spot that hides a defect. Security: no new surface. Selected transcript text already reached the terminal through this path on macOS and through `/copy` everywhere; the same bytes now travel on the other platforms, on a user-initiated drag, which is what a native terminal selection would have done anyway. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues N/A --------- Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Summary
Two presentation defects in the TUI chat transcript. They ship together because both
change what the virtualized transcript has to reserve rows for, and reviewing the row
arithmetic once is safer than twice.
The cover was never a screen. It is transcript row 0 -- a message with
kind: 'intro'that
appLayoutrenders as the wordmark plus the session panel -- andcapHistorypinnedit at index 0, so it was exempt even from the history cap. Measured against the real
renderer it occupies 35 terminal rows, while
estimatedMsgHeightguessed 9. So the firstanswer was appended below a full screen of cover art instead of replacing it, and
scrolling back up always found it again.
hideIntroAfterFirstTurndrops it from the view once auserorassistantrow exists.The row itself stays in
historyItems, because the latesession.infoevent patchesitself onto that row, and
/exportand the slash handlers read the session's own listrather than whatever is on screen. The trigger is deliberately a turn and not "any other
row": startup notices go through
sys()asrole: 'system'and slash output iskind: 'panel', so gating on those would mean a host that warns about its credentials onboot never shows a cover at all.
Both the rows and the layout's copy come from the one filtered array.
appLayoutcomparesrow.indexagainst indices it derives fromtranscript.historyItems, so filtering one andnot the other would move the inter-turn separator and the todo panel one row off.
The person's own message now renders on a filled background, so a turn reads as an
inserted card rather than one more line of prose.
userBgis a curated literal per tierand scheme, the way the rest of the reduced-tier palette is carried, and is skinnable
through
ui_user_bg. Below 256 colors there is no shade between black and brightBlack, sothe fill is skipped and the prompt chevron carries the row alone. The two padding rows are
drawn at every tier so
estimatedMsgHeightcan reserve a row count without reading theterminal's color capability; it now reserves four rows for a user message rather than two.
One testing note worth carrying forward:
ink-testing-librarybrings its own reconcilerand silently drops both foreground colors and background fills, so a fill is only
observable through
renderSyncwriting real escape codes to a stream. An early version ofthese tests passed against nothing.
Type
Mixed in practice: the cover half is a fix, the filled block is new behaviour.
Verification
Run in a clean worktree at this branch, with
packages/hermes-inkbuilt from this baserather than borrowed, so the render assertions exercise the Ink this branch ships:
npm run type-check-- clean.npx eslint src/-- 0 errors, 22 warnings, all pre-existing.useMainApp.tscarries twoof them before and after the change (same rules, shifted 12 lines by the inserted
comment), confirmed by linting the pristine file from
mainand diffing.npx vitest run --no-file-parallelism-- 87 files, 996 passed, 13 skipped, 0 failed.scripts/check_commit_messages.py main..HEAD-- exit 0.theatre: neutering
hideIntroAfterFirstTurnreddens the two cover tests; removing onlythe
backgroundColorprop while keeping the padding reddens the fill test; changing thereserved rows from four back to two reddens both height tests.
Suites that exercise the changed code directly:
messages.test.ts(the filter, and theblock rendered through
renderSync),virtualHeights.test.ts(the reserved rows),theme.test.ts(the token per tier, and the fill gate).ui-tui/CONTEXT.mdgains Prompt Block and Cover, since both are named in the codeand neither existed as a documented term.
Risk
Visible change to every chat transcript: a user message grows from three rows to five (two
of them the filled padding), and the cover stops being visible after the first turn.
Nothing is removed from the session -- the cover row is still in
historyItems, so/exportand session save are byte-identical.Rollback is the revert of this one commit; there is no migration, no persisted state, and
no wire-format change. A skin that dislikes the shade can set
ui_user_bgwithout a codechange.
Known gaps, disclosed rather than left for review to find:
virtualRowsand the layout'shistoryItemsis held byconstruction and by a comment, not by a test; there is no existing harness that mounts
useMainAppagainst a gateway.theme.tscitesscripts/gen-color-palettes.mjsas the provenance of the reduced-tierpalettes and a
docs/tui-color-problem/tokens.mdfor the tier-2 values. Neither holdstoday: that script's embedded source palette is a stale green one, its algorithm maps
#161b22toansi256(16)where the file hasansi256(234), and the doc does not exist.The values are hand-curated, and the ones added here follow the existing neighbours. Both
stale comments are left alone rather than widen this diff.
Related Issues
N/A