Skip to content

fix(tui): avoid mutating stdout dimensions - #35909

Closed
sxuff wants to merge 1 commit into
NousResearch:mainfrom
sxuff:fix/tui-stdout-dimensions
Closed

fix(tui): avoid mutating stdout dimensions#35909
sxuff wants to merge 1 commit into
NousResearch:mainfrom
sxuff:fix/tui-stdout-dimensions

Conversation

@sxuff

@sxuff sxuff commented May 31, 2026

Copy link
Copy Markdown

What does this PR do?

Replaces the global process.stdout.columns / process.stdout.rows monkey-patch with safe terminal dimension accessors.

The previous approach used Object.defineProperty() on process.stdout, which can break wrapped stdout implementations and cause Ink layout issues in terminals such as Warp. This PR keeps the existing terminal dimension sanitization behavior, but applies it at dimension read sites instead of mutating the host stream.

It also makes dimension reads defensive against unusual stdout wrappers and adds focused test coverage for invalid, oversized, missing, fractional, and throwing terminal dimension sources.

Related Issue

Fixes #35738

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • Removed the startup stdout dimension monkey-patch from ui-tui/src/entry.tsx.
  • Added safe dimension accessors in ui-tui/src/lib/terminalDimensions.ts.
  • Updated TUI dimension reads to use safeColumns() / safeRows().
  • Added package-local safe dimension accessors for vendored Ink in ui-tui/packages/hermes-ink/src/ink/terminal-dimensions.ts.
  • Updated Ink renderer dimension reads to sanitize without mutating stdout.
  • Added focused tests in:
    • ui-tui/src/__tests__/terminalDimensions.test.ts
    • ui-tui/packages/hermes-ink/src/ink/terminal-dimensions.test.ts

How to Test

  1. cd ui-tui
  2. npm.cmd exec -- vitest run src/__tests__/terminalDimensions.test.ts packages/hermes-ink/src/ink/terminal-dimensions.test.ts
  3. npm.cmd run build --prefix packages/hermes-ink
  4. npm.cmd run build

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Focused terminal dimension tests pass:

Test Files  2 passed (2)
Tests       24 passed (24)

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tui Terminal UI (ui-tui/ + tui_gateway/) javascript labels May 31, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Competes with open #35766 (WSL env var gating) and #35764 (TERM_PROGRAM-based detection) — all three fix #35738 (clampStdoutDimensions breaking Warp terminal). This PR takes the most comprehensive approach by removing the global mutation entirely.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Replaces the global process.stdout.columns/rows monkey-patch (which broke Ink layout in Warp) with read-site sanitization via new safeColumns/safeRows/safeTerminalSize helpers. The helpers defensively read dimensions (with try/catch for throwing getters) and sanitize values without mutating the host stream.

Changes:

  • Removed clampStdoutDimensions and its startup invocation in entry.tsx; added safeColumns/safeRows/safeTerminalSize accessors in ui-tui/src/lib/terminalDimensions.ts.
  • Replaced all stdout?.columns ?? 80 / stdout?.rows ?? 24 reads in TUI app and overlay components with the safe accessors; added a parallel helper inside the vendored Ink package and wired its constructor/resize/render paths.
  • Added focused tests for both helpers covering invalid, oversized, missing, fractional, throwing, and fallback cases.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file
File Description
ui-tui/src/lib/terminalDimensions.ts Removes clampStdoutDimensions; adds safeColumns/safeRows/safeTerminalSize with defensive reads and fallback sanitization.
ui-tui/src/entry.tsx Drops the startup stdout monkey-patch.
ui-tui/src/app/useMainApp.ts Uses safeColumns(stdout) for cols state and resize RPC.
ui-tui/src/app/useInputHandlers.ts Uses safeRows(terminal.stdout) for pager / page-up viewport.
ui-tui/src/components/{branding,modelPicker,sessionPicker,skillsHub,activeSessionSwitcher,agentsOverlay}.tsx Switches direct stdout dimension reads to the safe accessors.
ui-tui/src/tests/terminalDimensions.test.ts Replaces clamping tests with safe-reader coverage.
ui-tui/packages/hermes-ink/src/ink/terminal-dimensions.ts New package-local mirror of the sanitizer + safe readers.
ui-tui/packages/hermes-ink/src/ink/terminal-dimensions.test.ts Vitest coverage for the package-local helpers.
ui-tui/packages/hermes-ink/src/ink/ink.tsx Uses safeTerminalSize/safeColumns/safeRows in constructor, resize handler, and render path.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Replaces the global stdout columns/rows monkey-patch with safe terminal dimension accessors. This avoids mutating wrapped stdout streams, which can break Ink layout in terminals such as Warp.

Validation: focused terminal dimension tests and TUI builds.
@teknium1

teknium1 commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Thanks for this, @sxuff — solid work and a real cross-platform concern.

Closing as redundant against current main. The Object.defineProperty(process.stdout, ...) monkey-patch this PR removes was already ripped out: the whole terminal-dimension clamp (#35657) was reverted in #36096, so the patch, terminalDimensions.ts, and its tests no longer exist on main. This branch is also conflicting against current main as a result.

Separately, the original WSL2 symptom that motivated all this — columns=131072, rows=1 causing blank/disappearing text — was reported on the Web dashboard /chat tab, which is a Python path (xterm.js → WebSocket → PtyBridge), not the Node/Ink TUI either of these PRs touched. We've fixed that server-side by clamping dimensions in PtyBridge.resize() before they reach struct.pack (which packs as unsigned short, so 131072 was raising an uncaught struct.error): #38200.

Appreciate the careful test coverage on this PR — the read-site sanitization approach is sound and we may revisit it for the Ink TUI surface if the WSL garbage value resurfaces there.

@teknium1 teknium1 closed this Jun 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(tui): clampStdoutDimensions (#35657) breaks TUI layout in Warp terminal

4 participants