fix(tui): suppress verbose logs while alt-screen is active by Sskift · Pull Request #2262 · Hmbown/CodeWhale

Sskift · 2026-05-27T06:29:47Z

Summary

track the user-requested verbose setting separately from the currently-active verbose state
suppress verbose CLI logging while the TUI owns the alt-screen on Windows and restore it on every exit path
cover the new alt-screen verbosity transitions with focused logging tests

Closes #1909.

Testing

tmpdir=$(mktemp -d) && HOME="$tmpdir" USERPROFILE="$tmpdir" RUSTUP_HOME="/Users/bytedance/.rustup" CARGO_HOME="/Users/bytedance/.cargo" cargo test -p codewhale-tui logging::tests::
tmpdir=$(mktemp -d) && HOME="$tmpdir" USERPROFILE="$tmpdir" RUSTUP_HOME="/Users/bytedance/.rustup" CARGO_HOME="/Users/bytedance/.cargo" cargo test -p codewhale-tui

Greptile Summary

This PR fixes verbose log output leaking into the Windows TUI alt-screen by splitting the single VERBOSE atomic into three: REQUESTED_VERBOSE (what the user asked for), IN_ALT_SCREEN (current TUI state), and VERBOSE (the live effective value derived from the other two). Previous review feedback on set_verbose bypass and parallel test races has been fully addressed.

logging.rs gains suppress_for_tui_alt_screen / restore_after_tui_alt_screen helpers and a pure alt_screen_verbose_state combinator; stateful tests are serialized with a shared Mutex and a start-of-test reset_logging_state() call that correctly survives lock poisoning.
tui/ui.rs wires the new helpers into every alt-screen entry and exit site: the normal run_tui unwind path, TerminalCleanupGuard::drop (panic/early-return safety net), pause_terminal/resume_terminal (subprocess pauses), and emergency_restore_terminal.

Confidence Score: 5/5

Safe to merge — the change is self-contained to logging state management with no impact on TUI rendering or data paths.

All exit paths (normal unwind, panic via Drop, subprocess pause/resume, emergency restore) correctly call restore_after_tui_alt_screen, the suppress/restore pair is idempotent, and the test suite serializes the stateful global-atomic tests while leaving the pure-function tests free to run concurrently. No regressions are apparent on non-Windows platforms since alt_screen_verbose_state is a no-op when is_windows is false.

No files require special attention.

Important Files Changed

Filename	Overview
crates/tui/src/logging.rs	Adds REQUESTED_VERBOSE and IN_ALT_SCREEN atomics to track the user-requested verbose setting separately from the live VERBOSE state; introduces suppress_for_tui_alt_screen/restore_after_tui_alt_screen helpers and a pure alt_screen_verbose_state combinator; tests are serialized via a shared Mutex with reset at the start of each stateful test, correctly recovering from lock poisoning.
crates/tui/src/tui/ui.rs	Hooks suppress_for_tui_alt_screen / restore_after_tui_alt_screen into every alt-screen entry and exit point (run_tui normal path, TerminalCleanupGuard::drop, pause_terminal/resume_terminal, and emergency_restore_terminal), covering all exit paths including panics via the drop guard.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant run_tui
    participant logging
    participant TerminalCleanupGuard

    Caller->>run_tui: run_tui(config, options)
    run_tui->>run_tui: execute!(EnterAlternateScreen)
    run_tui->>logging: suppress_for_tui_alt_screen()
    note over logging: IN_ALT_SCREEN=true<br/>VERBOSE=false (Windows)<br/>REQUESTED_VERBOSE unchanged

    run_tui->>run_tui: "create TerminalCleanupGuard (defused=false)"

    alt Subprocess pause/resume
        run_tui->>run_tui: pause_terminal()
        run_tui->>logging: restore_after_tui_alt_screen()
        note over logging: IN_ALT_SCREEN=false<br/>VERBOSE=REQUESTED_VERBOSE
        run_tui->>run_tui: resume_terminal()
        run_tui->>logging: suppress_for_tui_alt_screen()
    end

    alt Normal exit
        run_tui->>run_tui: "cleanup_guard.defused = true"
        run_tui->>run_tui: execute!(LeaveAlternateScreen)
        run_tui->>logging: restore_after_tui_alt_screen()
        note over logging: IN_ALT_SCREEN=false<br/>VERBOSE=REQUESTED_VERBOSE
    else Panic / early return
        TerminalCleanupGuard->>run_tui: "drop() [defused=false]"
        run_tui->>run_tui: execute!(LeaveAlternateScreen)
        TerminalCleanupGuard->>logging: restore_after_tui_alt_screen()
    else Emergency
        Caller->>run_tui: emergency_restore_terminal()
        run_tui->>logging: restore_after_tui_alt_screen()
    end

_{Reviews (5): Last reviewed commit: "test(logging): recover poisoned test mut..." | Re-trigger Greptile}

gemini-code-assist · 2026-05-27T06:30:01Z

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

Hmbown

Approved after the follow-up test-only commit 4e2b6a6.

I checked the alt-screen suppression paths in logging.rs/ui.rs and pushed a small test cleanup so set_verbose_respects_alt_screen_suppression now asserts the Windows and non-Windows expectations directly instead of comparing against the same helper.

Local verification on the updated head:

cargo fmt --all -- --check
git diff --check
cargo test -p codewhale-tui logging::tests:: -- --nocapture
cargo check -p codewhale-tui --all-features --locked

Leaving merge to the normal gate; GitHub CI is still running after the latest push.

Hmbown · 2026-05-27T11:28:10Z

Pushed a small maintainer follow-up for the test-reliability review note: the serialized logging-state tests now recover a poisoned mutex guard with into_inner(), so a real assertion failure does not cascade into unrelated "poisoned lock" failures.

Local verification on the updated head:

cargo test -p codewhale-tui logging::tests:: -- --nocapture
cargo fmt --all -- --check
git diff --check

Hmbown

The maintainer follow-up keeps the test mutex from poisoning across assertions, and the refreshed CI matrix is now clean across Ubuntu, macOS, Windows, npm smoke, GitGuardian, and Greptile. This is a focused v0.8.48 candidate for the Windows alt-screen verbose-log leak path. Thanks @Sskift for carrying the regression coverage here.

Hmbown · 2026-05-27T13:15:24Z

Independent review (Devin):

Correctness — suppress/restore coverage is complete. All five alt-screen transitions call the right hook: run_tui enter/leave (ui.rs:286, 558), TerminalCleanupGuard::drop (line 621), pause_terminal/resume_terminal (lines 6913, 6934), and emergency_restore_terminal (line 7048). The defused flag on the guard prevents the only double-restore path.

Non-atomic TOCTOU in set_verbose (logging.rs:26–29): REQUESTED_VERBOSE.store → IN_ALT_SCREEN.load → VERBOSE.store are three separate SeqCst ops. A concurrent suppress_for_tui_alt_screen call on another thread (unlikely today — set_verbose is only called from main.rs:801 at startup) could leave VERBOSE=true on Windows after the suppress. Not a real-world risk given the single call-site, but worth a comment in the code.

Suppression is Windows-only; IN_ALT_SCREEN is stored unconditionally on all platforms. That's correct but suppress_for_tui_alt_screen / restore_after_tui_alt_screen are no-ops on non-Windows (VERBOSE unchanged). A brief doc comment on the public functions would clarify intent for future maintainers.

Test serialisation is solid. into_inner() on a poisoned lock guard prevents cascading failures across the logging-state tests (Hmbown follow-up).

v0.8.48 (#2256) compatibility: clean. The feature branch touches runtime_log.rs, runtime_threads.rs, and distinct sections of tui/ui.rs (build_engine_config, switch_provider, render); no overlap with this PR's changes to logging.rs or the alt-screen hooks in ui.rs.

Hmbown · 2026-05-31T04:11:56Z

Thanks @Sskift — this still looks like the right fix for #1909, and the Windows CI signal is exactly what I wanted to see here.

The blocker now is mechanical: GitHub can’t create a clean merge commit against current main. Please rebase this branch on the latest main and keep the focused logging tests green; after that I expect this to be mergeable and we can close #1909 with proper credit to @aboimpinto for the original report.

Hmbown · 2026-05-31T05:06:36Z

Thanks @Sskift — this was the right shape of fix for the Windows alt-screen logging leak, and the tests/review work here helped validate the behavior.

I’m closing this as superseded because #1909 is now fixed by #2295 (bed328b7) on main, with the same user-facing outcome: verbose runtime logging is suppressed while the TUI owns the alt-screen and restored after exit/resume. I credited the original report on the issue closure note, and this PR remains useful review context for any future Windows alt-screen logging follow-up.

fix(tui): suppress verbose logs while alt-screen is active

2e58d47

greptile-apps Bot reviewed May 27, 2026

View reviewed changes

Comment thread crates/tui/src/logging.rs

fix(logging): keep alt-screen suppression in set_verbose

e0ed1db

greptile-apps Bot reviewed May 27, 2026

View reviewed changes

Comment thread crates/tui/src/logging.rs

liushiao and others added 2 commits May 27, 2026 17:48

test(logging): serialize global-state verbose tests

ce80758

test(logging): assert alt-screen verbose state explicitly

4e2b6a6

Hmbown mentioned this pull request May 27, 2026

TUI 对话中进程崩溃，输入内容泄漏到 PowerShell 终端 #2261

Open

Hmbown approved these changes May 27, 2026

View reviewed changes

test(logging): recover poisoned test mutex

fa54961

Hmbown approved these changes May 27, 2026

View reviewed changes

This was referenced May 27, 2026

fix(tui): init runtime log before alt-screen, add Windows stderr redirect (#1909) #2259

Merged

fix(tui): suppress verbose CLI logging on Windows alt-screen to prevent TUI leak #1910

Closed

Hmbown closed this May 31, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(tui): suppress verbose logs while alt-screen is active#2262

fix(tui): suppress verbose logs while alt-screen is active#2262
Sskift wants to merge 5 commits into
Hmbown:mainfrom
Sskift:fix/issue-1909-windows-verbose-alt-screen

Sskift commented May 27, 2026 •

edited by greptile-apps Bot

Loading

Uh oh!

gemini-code-assist Bot commented May 27, 2026

Uh oh!

Uh oh!

Uh oh!

Hmbown left a comment

Uh oh!

Hmbown commented May 27, 2026

Uh oh!

Hmbown left a comment

Uh oh!

Hmbown commented May 27, 2026

Uh oh!

Hmbown commented May 31, 2026

Uh oh!

Hmbown commented May 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Sskift commented May 27, 2026 • edited by greptile-apps Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

gemini-code-assist Bot commented May 27, 2026

Uh oh!

Uh oh!

Uh oh!

Hmbown left a comment

Choose a reason for hiding this comment

Uh oh!

Hmbown commented May 27, 2026

Uh oh!

Hmbown left a comment

Choose a reason for hiding this comment

Uh oh!

Hmbown commented May 27, 2026

Uh oh!

Hmbown commented May 31, 2026

Uh oh!

Hmbown commented May 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Sskift commented May 27, 2026 •

edited by greptile-apps Bot

Loading