feat(tui): trial an anchored composer in fullscreen mode on nightly - #4221
feat(tui): trial an anchored composer in fullscreen mode on nightly#4221abhinav-phi wants to merge 2 commits into
Conversation
…pache#4136) Opt the TUI into an alternate-screen fullscreen renderer on nightly builds: the transcript scrolls in an application-owned viewport while the composer, activity strip, pending queue, and status line stay anchored to the bottom of the screen. MAKA_TUI_FULLSCREEN=1 opts a release build in; =0 opts a nightly build out. - TuiAltScreen with mouse wheel scrolling, drag selection + OSC 52 copy, transcript search (Ctrl+Shift+F), and clickable OSC 8 links - primary ScrollView follows the newest output and preserves the reading position while scrolled away - an unread indicator counts lines appended while away and clears on return; typing re-anchors to the newest output - app-owned viewport disables the main-screen scrollback entry freeze, so expansion toggles retarget every entry
hqhq1025
left a comment
There was a problem hiding this comment.
Codex-assisted review performed under the maintainer-approved review workflow.
Reviewed exact head 285c6039155929b616542f36e4cb802ca02571d5. This change enables the alternate-screen TUI by default for nightly builds, threads build identity into the runner, adds a transcript ScrollView with anchored editor/status chrome and unread tracking, and enables mouse selection/search/link activation. I inspected the complete 8-file diff, the version-selection path, layout/scroll state, editor input routing, hyperlink activation, terminal teardown, and the added tests.
I found one P1 security issue in the Windows hyperlink opener; see the inline comment.
Validation: the exact-head maka-agent suite passed 659/659; a clean synthetic merge with current main at d2346707d65144682d45e905a378ee57be469769 also passed 659/659. Biome on all changed files, ASF header validation, and git diff --check passed. GitHub currently exposes only the successful PR-effort label check, not the repository test workflow.
Not independently verified: native Windows execution, real-terminal OSC 52 selection behavior, and long-session performance. This feature-level rollout still requires human product/merge judgment after the security issue is fixed.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| return; | ||
| } | ||
| if (platform === 'win32') { | ||
| spawn('cmd', ['/c', 'start', '', url], { |
There was a problem hiding this comment.
[P1] Do not pass model-authored link targets through cmd.exe. Assistant Markdown is rendered as OSC 8 with the raw href, and a click forwards that value here. With windowsVerbatimArguments: false, an argument such as https://example.com/?x=1&calc.exe is not quoted merely because it contains &, so cmd /c can interpret &calc.exe as another command. That makes a displayed assistant link a click-triggered command-execution path on Windows. Please restrict accepted protocols (the desktop already allows only http:, https:, and mailto:) and use a platform opener that does not parse the target as shell syntax; add a Windows-focused regression covering &, |, %, quotes, and rejected schemes.
There was a problem hiding this comment.
Fixed in ce164e6 — thank you for catching this; the finding is exactly right (cmd /c start + spawn's non-escaping argument quoting = click-triggered command execution from a model-authored href).
The fix, matching both parts of the recommendation:
- Protocol allowlist —
openExternalUrlnow parses the target withnew URLand hands off onlyhttp:,https:, andmailto:, the same allowlist as the desktop's external-link guard (apps/desktop/src/main/external-link-guard.ts).file:,javascript:,ftp:, unknown handlers, UNC paths, and malformed targets are ignored without spawning anything.URLnormalizes schemes to lowercase, so casing can't smuggle past it. - Shell-free Windows opener — the
cmd /c startpath is gone. Windows now spawnsrundll32 url.dll,FileProtocolHandler <url>directly: the URL is a single argv element that never reaches cmd.exe (so&,|,%, and quotes are inert), and the DLL/entrypoint half of the command line is a compile-time constant, so a hostile URL cannot redirect what runs. macOS/Linux keepopen/xdg-openwith the URL as a plain argv element (no shell there either).
Regression tests added in tui-fullscreen.test.ts (opener is now spawn-injectable so tests never touch real processes):
- hostile payloads
https://example.com/?x=1&calc.exe,?x=1|calc.exe,?x=%PATH%, and?q="quoted"&x=1onwin32assert exactly one spawn —rundll32with['url.dll,FileProtocolHandler', <url>], nevercmd; - nine rejected targets (
file:with an absolute path,javascript:,ftp:,calc://,ms-msdt:,\\server\share, unparseable text, empty, trailing text) × three platforms assert zero spawns; mailto:and an uppercase-schemeHTTPS://URL still open (scheme normalization), and macOS/Linux receive the URL as plain argv.
biome, the ASF header check, tsc across all workspaces, and the fullscreen/runner suites (28 unit + 2 runner-level tests) pass on the new head.
Review finding on apache#4221: assistant Markdown renders OSC 8 links with the raw href, and the Windows opener passed that href through 'cmd /c start', where spawn's argument quoting does not escape shell metacharacters — a link like https://example.com/?x=1&calc.exe could start a second command under cmd.exe. - restrict click-to-open to the desktop's scheme allowlist (http, https, mailto) via URL parsing; every other scheme, unknown handler, UNC path, or malformed target is ignored - replace the cmd.exe path with 'rundll32 url.dll,FileProtocolHandler': the URL stays a single argv element and never reaches a shell; the DLL/entrypoint half is a compile-time constant so a hostile URL cannot redirect it - regression tests cover &, |, %, quotes, rejected schemes, and the argv-passed macOS/Linux openers
|
The P1 from the automated review is fixed in ce164e6:
Biome, ASF headers, |
Summary
Implements the nightly trial proposed in #4136: the TUI can now run fullscreen (alternate screen) with an anchored composer — the prompt, pending queue, activity strip, and status line stay pinned to the bottom of the screen while the transcript scrolls in an application-owned viewport. On nightly builds the fullscreen path is the default; release builds keep today's terminal-scrollback renderer.
MAKA_TUI_FULLSCREEN=1opts any build in,MAKA_TUI_FULLSCREEN=0opts a nightly build out.This builds on the capability analysis in Discussion #3879: the pinned
@earendil-works/pi-tui@0.84.2already shipsTuiAltScreen(alternate-screen viewport with mouse, selection, search, and hyperlink support), so this is a mode switch inside the existing dependency — no renderer fork, no upstream rewrite.What changes
packages/cli/src/tui-fullscreen.ts(new) — the experiment switch, the unread-output logic, and the hardened link opener:resolveTuiFullscreen()resolves the mode with precedence: explicit setting (embeddings/tests) →MAKA_TUI_FULLSCREENenv override → build-channel default. Nightly is detected from the CLI package version using the Product Nightly identity format (0.2.0-dev.<run>.<YYYYMMDD>).UnreadOutputCounter— counts transcript lines appended while the user is scrolled away from the bottom; cleared on return to the bottom, never negative on content shrink.openExternalUrl()— click-to-open for OSC 8 hyperlinks. Assistant hrefs are model-authored (untrusted input), so onlyhttp:/https:/mailto:targets are handed off (the same allowlist as the desktop's external-link guard,apps/desktop/src/main/external-link-guard.ts), and openers never pass the URL through a shell: Windows usesrundll32 url.dll,FileProtocolHandler(the URL stays a single argv element; the DLL/entrypoint half is a compile-time constant), macOS/Linux pass it as a plain argv element toopen/xdg-open.packages/cli/src/pi-tui-layout.ts— the fullscreen layout pieces:MakaTranscriptScrollView— aScrollView(follow-end, primary, chaining overscroll, transient scrollbar) that computes the unread count at its layout pass — the one point in each frame where scroll state is fresh — and requests a catch-up frame when the rendered count lags, so the indicator settles deterministically.MakaTranscriptDocumentComponent— renders the full transcript document inside the scroll view and exposes its line count.MakaFullscreenChromeComponent— the anchored bottom chrome (unread indicator, activity strip, pending queue, editor, status line) with the same editor/autocomplete row-budget fixed-point as the main-screen layout, plus a minimum reserved transcript row.packages/cli/src/pi-tui-runner.ts— when fullscreen is on, constructsTuiAltScreenwithmouse: trueand the URL opener, mounts theVStacklayout root (scrolling transcript + intrinsic-height chrome), and wires the composer; the main-screen layout and its clear-on-shrink protection are skipped entirely.packages/cli/src/skill-highlight-editor.ts— adds anonUserTextChangedhook fired after any input that actually changes the editor text; fullscreen uses it to re-anchor the transcript to the newest output while typing.packages/cli/src/cli-core.ts/runtime-host-tui-command.ts— thread the CLI package version to the runner so the channel default can be resolved.The fullscreen experience
PageUp/PageDown, andCtrl+Shift+↑/↓(semantic prompt jumps) scroll the transcript; the editor and status line never move.↓ N new lines — End to jump to latest) appears between the transcript and the composer, counting lines appended while away;End(or scrolling back down) clears it.Ctrl+Shift+Fsearches the rendered transcript with next/previous match navigation.Behavior decisions (mapped to the issue's evaluation questions)
Endjump make catching up one keystroke. Whether it materially helps is exactly what nightly feedback should answer.Enabling the experiment
0.2.0-dev.*)MAKA_TUI_FULLSCREEN=0to opt out0.2.0)MAKA_TUI_FULLSCREEN=1to opt inThis satisfies the issue's non-goals by construction: fullscreen is not the stable default on release builds, there is no permanent user-facing mode toggle (only the experiment env var, the same pattern as
MAKA_RUNTIME_SAFE_BOUNDARY_RESUME), and the upstream renderer is untouched.Exit criteria — what to try and report on nightly
End/Homenote: in the alternate screen these keys jump to the bottom/top of the transcript (pi-tui's intentional shadowing of the editor's line navigation);Ctrl+E/Ctrl+Astill move the cursor.MAKA_TUI_FULLSCREEN=0), so a bad nightly can be worked around without a revert.Testing
packages/cli/src/__tests__/tui-fullscreen.test.ts(new, 28 tests): the mode-resolution matrix (setting/env/channel), unread-counter semantics (growth while away, clear at bottom, shrink safety, reset), indicator copy, chrome sizing/reserved rows/activity-separator, fullrenderLayoutFrameintegration frames — composer anchoring, follow-end behavior, reading-position preservation under growth, and the exact two-frame unread convergence — and the link-opener hardening regressions.&,|,%, and quoted metacharacter payloads on Windows always reach the shell-freerundll32opener as a single argv element (nevercmd.exe); every non-allowlisted scheme (file:,javascript:,ftp:, unknown handlers, UNC paths, malformed targets) spawns nothing on any platform;mailtoand uppercase-scheme URLs still open; macOS/Linux targets are passed as plain argv.packages/cli/src/__tests__/pi-tui-runner.test.ts(+2 runner-level tests on theFakeTerminalharness): a tall resumed session proves wheel-up keeps the composer anchored while older content scrolls in and typing re-anchors to the newest output; and a four-way gating run (release/nightly × env override) asserts which renderer actually starts.biome check, ASF header check, andtsctypecheck across all workspaces pass. The CLI suite matches the pre-existingmainbaseline on this machine (the only divergent test was a flaky MCP-child process test that passes in isolation and is untouched by this change).Rollout
Nightly builds pick this up automatically; the issue's exit criteria (documented feedback on reading position, unread indication, selection/copy, terminal history, resizing, small terminals) then decide whether the mode is stabilized, revised, or dropped.