Skip to content

mux: read-screen reports the rendered viewport (fixes the macOS smoke-attach banner divergence) - #7488

Merged
lawrencecchen merged 2 commits into
mainfrom
fix-mux-attach-banner-race
Jul 7, 2026
Merged

lawrencecchen merged 2 commits into
mainfrom
fix-mux-attach-banner-race

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Jul 7, 2026 •

Copy link
Copy Markdown
Contributor

Fixes main's persistent test (macos) failure in scripts/smoke-attach.py (assert_client_matches_server), which reproduces deterministically with SHELL=/bin/bash scripts/smoke-attach.py (3/3 before, 5/5 pass after).

Root cause (verified in code by the /fable judge, not just observed): the attach mirror was correct; the comparison oracle was wrong. Terminal::plain_text() formats the active screen's full page list including scrollback (null-selection ScreenFormatter over the .screen point space), while clients render the RenderState viewport. When macOS bash's "default interactive shell is now zsh" banner scrolled into scrollback and \e[H\e[2J repainted the prompt, read-screen kept reporting the stale banner rows. The spec already promised read-screen output "does not include prior scrollback", so the old behavior violated its own contract.

Fix: read-screen uses a new Terminal::viewport_text() built on the terminal formatter's read-only selection path (GHOSTTY_POINT_TAG_VIEWPORT, (0,0)..(cols-1,rows-1), unwrap:false). A first-round RenderState::update approach was rejected in review because that snapshot consumes per-row dirty flags and would starve a concurrent TUI renderer's row refresh in non-headless sessions; viewport_text_does_not_clear_render_dirty_state locks in the read-only property.

Two-commit red/green: commit 1 adds the failing socket regression (scrollback + clear-screen → viewport reporting), commit 2 the fix. No ghostty submodule changes (the formatter options already existed in the C API).

Verified: cargo fmt/clippy/test workspace-clean (121 tests), smoke-tui, SHELL=/bin/bash smoke-attach.py 5/5. /fable judge: APPROVE after one REVISE round.


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.


Note

Medium Risk
Changes the observable read-screen contract (intentionally aligning with spec); low blast radius but affects attach/smoke oracles and any client that assumed scrollback-inclusive text.

Overview
read-screen now returns the rendered viewport instead of plain_text(), which still dumps the active screen including scrollback. That mismatch caused attach/smoke comparisons to see stale rows (e.g. macOS bash banner) after content scrolled away and the screen was cleared.

Adds Terminal::viewport_text(), formatting the full viewport via the terminal formatter’s read-only selection path (GHOSTTY_POINT_TAG_VIEWPORT, unwrap: false) so inspection does not consume RenderState dirty flags. Selection helpers gain selection_text_with_tag_options for configurable unwrap/trim; plain_text() docs now point callers at viewport_text() for viewport-only dumps.

Regression coverage: viewport_text_does_not_clear_render_dirty_state and a control-socket test (scrollback + clear-screen → prompt on first line).

Reviewed by Cursor Bugbot for commit 40347a8. Bugbot is set up for automated code reviews on this repo. Configure here.


Summary by cubic

read-screen now returns the currently rendered viewport (not scrollback). This fixes the macOS smoke-attach mismatch and aligns with the command spec.

  • Bug Fixes
    • mux-core: read-screen now calls Terminal::viewport_text() instead of plain_text().
    • ghostty-vt: Added viewport_text() using the read-only formatter over GHOSTTY_POINT_TAG_VIEWPORT; it does not clear render dirty flags.
    • Tests: Added viewport_text_does_not_clear_render_dirty_state and a control socket test to verify viewport output after scrollback + clear-screen.

Written for commit 40347a8. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • New Features

    • Added a way to read only the currently visible terminal viewport as plain text.
    • Screen-read responses now return the rendered viewport instead of the full scrollback history.
  • Bug Fixes

    • Improved screen-reading behavior after clearing scrollback so the returned text matches what is actually on screen.
    • Confirmed viewport reads do not affect the terminal’s render-dirty state.

…ewport

With scrollback present and a clear-screen repaint, read-screen
reported stale scrollback rows instead of the rendered viewport,
diverging from what attach clients render. Test only; fix follows.
Terminal::plain_text() formats the active screen's full page list
including scrollback (null-selection ScreenFormatter over .screen
point space), while attach clients render the RenderState viewport.
After macOS bash's zsh-advice banner scrolled rows into scrollback
and the prompt repainted, read-screen reported the stale banner rows
and smoke-attach's client/server comparison diverged persistently:
main's test (macos) red, reproducible locally with SHELL=/bin/bash.
The spec (mux/spec/commands.md) already promised read-screen output
'does not include prior scrollback', so this honors the contract.

read-screen now uses a new Terminal::viewport_text(): the terminal
formatter's read-only selection path over GHOSTTY_POINT_TAG_VIEWPORT
(0,0)..(cols-1,rows-1) with unwrap:false, one line per visual row.
RenderState::update was rejected for this because it consumes per-row
dirty flags and would starve a concurrent TUI renderer's row refresh
in non-headless sessions; a regression test locks in the read-only
property (viewport_text_does_not_clear_render_dirty_state).
@vercel

vercel Bot commented Jul 7, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment Jul 7, 2026 4:07am
cmux-staging Building Building Preview, Comment Jul 7, 2026 4:07am

@coderabbitai

coderabbitai Bot commented Jul 7, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 80f32ac1-18dc-4b86-8389-75480a8ec77f

📥 Commits

Reviewing files that changed from the base of the PR and between adc4887 and 40347a8.

📒 Files selected for processing (4)
  • mux/crates/ghostty-vt/src/terminal.rs
  • mux/crates/ghostty-vt/tests/terminal.rs
  • mux/crates/mux-core/src/server.rs
  • mux/crates/mux-core/tests/pty.rs

📝 Walkthrough

Walkthrough

Adds a viewport_text method to Terminal that returns plain text for the currently rendered viewport, generalizes the internal selection-text formatter with unwrap_lines/trim options, updates plain_text docs, switches the mux-core read-screen command to use viewport_text, and adds corresponding unit and integration tests.

Changes

Viewport Text Feature

Layer / File(s) Summary
Selection formatter generalization and viewport_text
mux/crates/ghostty-vt/src/terminal.rs
Adds viewport_text() computing viewport bounds from cols/rows and delegating to a new selection_text_with_tag_options(unwrap_lines, trim); updates plain_text docs to clarify scrollback vs. viewport scope.
Terminal test coverage
mux/crates/ghostty-vt/tests/terminal.rs
Adds a test confirming viewport_text() does not clear RenderState dirty status.
mux-core read-screen wiring and PTY test
mux/crates/mux-core/src/server.rs, mux/crates/mux-core/tests/pty.rs
Changes the ReadScreen handler to call viewport_text() instead of plain_text(); adds an integration test verifying the control socket reports the rendered viewport after a scrollback clear.

Estimated code review effort: 2 (Simple) | ~12 minutes

Possibly related PRs

  • manaflow-ai/cmux#7180: Both PRs modify the mux control server's "read-screen" response path in mux-core/src/server.rs, with this PR switching the returned text source to viewport_text().
🚥 Pre-merge checks | ✅ 25
✅ Passed checks (25 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main functional change: read-screen now reports the rendered viewport instead of scrollback.
Description check ✅ Passed The description covers Summary, Testing, and Checklist, and is mostly complete despite missing the Demo Video and Review Trigger sections.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed Diff touches only Rust files; no production Swift code or actor-isolation-sensitive Swift APIs were introduced.
Cmux Swift Blocking Runtime ✅ Passed PASS: The PR changes only Rust files; no Swift files or Swift blocking/timing primitives were introduced, so the Swift-specific check is not applicable.
Cmux Browser Automation Off-Main ✅ Passed PR only changes Rust screen-reading path/tests; no .mainActor/worker-router browser automation code or browser.* routing was added or moved.
Cmux Expensive Synchronous Load ✅ Passed No Swift files changed in this PR; the diff is confined to Rust terminal/server code, so the Swift expensive sync-load rule is not applicable.
Cmux Cache Substitution Correctness ✅ Passed read-screen now reads the live viewport via a read-only formatter path, not a stale cache; the new test also verifies dirty flags stay intact.
Cmux No Hacky Sleeps ✅ Passed PASS: The only new waits are test-only polling in mux-core tests; production changes add read-only viewport_text/read-screen and no covered runtime-script sleeps.
Cmux Algorithmic Complexity ✅ Passed terminal.rs:299-312 uses viewport-sized selection; server.rs:543-547 switches read-screen to it. No nested scans/sorts on scalable collections.
Cmux Swift Concurrency ✅ Passed Diff only touches Rust files; no cmux-owned Swift sources were changed, so the Swift-concurrency rule is not applicable.
Cmux Swift @Concurrent ✅ Passed PR changes only Rust files; no Swift code was modified, so the @concurrent rule is not applicable.
Cmux Swift File And Package Boundaries ✅ Passed PR changes only Rust files/tests; no Swift files or Swift package-boundary issues are present, so the Swift rule is not applicable.
Cmux Swiftpm Lockfiles ✅ Passed PR diff only touches Rust source/tests; no .gitignore, workflow, Package.swift, or Package.resolved changes, so the SwiftPM lockfile rule is not triggered.
Cmux Swift Logging ✅ Passed PASS: The PR only changes Rust files; no Swift/runtime logging statements or review-rule violations were introduced.
Cmux User-Facing Error Privacy ✅ Passed The diff only changes read-screen to return viewport text and adds tests/docs; no new user-facing errors or output expose vendor/provider, secrets, or raw upstream details.
Cmux Full Internationalization ✅ Passed Only backend Rust logic/tests changed; no Swift/web copy or locale files were touched, and the new read-screen payload is terminal content, not localized UI text.
Cmux Swiftui State Layout ✅ Passed PASS: PR only changes Rust terminal/control-socket code; no SwiftUI views/state/layout code is introduced or modified.
Cmux Architecture Rethink ✅ Passed No Swift code changed; the Rust fix is a small correctness update with clear ownership/invariant and only test-only polling.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR only changes Rust terminal/server code; no Swift window/controller files or cmuxAuxiliaryWindowIdentifiers paths are touched.
Cmux Source Artifacts ✅ Passed All changed paths are Rust source/test files under mux/...; no logs, caches, temp dirs, screenshots, or other generated artifacts were added.
Cmux No Test Or Debug Seam In Production Source ✅ Passed No Swift production Sources files were changed; the PR only touches Rust crates/tests, so the no-test-debug-seam rule is not applicable.
Cmux No Ambient Global State ✅ Passed No Swift files changed; the rule targets production Swift code, and this diff only touches Rust files.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-mux-attach-banner-race

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR updates read-screen to report the rendered terminal viewport. The main changes are:

  • Adds Terminal::viewport_text() using the viewport selection formatter path.
  • Switches mux-core read-screen from full screen text to viewport text.
  • Adds tests for scrollback-cleared output and render dirty-state preservation.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.
  • The changed command behavior matches the documented viewport-only contract.
  • The tests cover the reported scrollback case and the render dirty-state invariant.

Important Files Changed

Filename Overview
mux/crates/ghostty-vt/src/terminal.rs Adds viewport-only terminal text formatting while keeping existing selection formatting behavior configurable.
mux/crates/mux-core/src/server.rs Routes the control-socket read-screen command through the new viewport text helper.
mux/crates/ghostty-vt/tests/terminal.rs Adds coverage that viewport text reads do not clear render dirty state.
mux/crates/mux-core/tests/pty.rs Adds a regression test for viewport output after scrollback and clear-screen behavior.

Reviews (1): Last reviewed commit: "mux: read-screen reports the rendered vi..." | Re-trigger Greptile

@lawrencecchen
lawrencecchen merged commit e157d80 into main Jul 7, 2026
49 of 53 checks passed

This branch was successfully deployed

1 active deployment
Preview – cmux — 40347a81 Deployed Jul 7, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant