fix(tui): init runtime log before alt-screen, add Windows stderr redirect (#1909) by Hmbown · Pull Request #2259 · Hmbown/CodeWhale

Hmbown · 2026-05-27T05:12:43Z

Problem

Verbose logging leaks into the TUI alt-screen on all platforms, especially severe on Windows where there was no stderr redirect at all.

Two root causes:

EnterAlternateScreen was called BEFORE runtime_log::init() — any logging between alt-screen entry and redirect init leaks raw bytes into the TUI buffer
redirect_stderr_to was #[cfg(unix)] only — Windows had no stderr redirect, so every eprintln!/tracing call during the TUI session wrote directly into the alt-screen

Fix

Swap order: init runtime_log (with stderr redirect) BEFORE entering the alt-screen, so the redirect is active from the start
Add Windows stderr redirect: #[cfg(windows)] redirect_stderr_to using SetStdHandle from the already-available windows crate, with corresponding Drop impl to restore the original handle

Verification

cargo check --workspace ✅
cargo test -p codewhale-tui ✅ — 3,449 passed, 0 failed
Windows code path uses the existing windows crate (already in Cargo.toml as a Windows dep)

Closes #1909

Greptile Summary

This PR fixes the TUI "scroll demon" on Windows and all platforms by (1) moving runtime_log::init() to run before EnterAlternateScreen so the stderr redirect is always active before alt-screen bytes flow, and (2) adding a #[cfg(windows)] redirect_stderr_to implementation using GetStdHandle/SetStdHandle to cover the platform that previously had no redirect at all.

crates/tui/src/tui/ui.rs: Swaps the order of runtime_log::init() and EnterAlternateScreen — the redirect is now established before any alt-screen escape sequences are written.
crates/tui/src/runtime_log.rs: Adds a Windows redirect_stderr_to function that saves the original stderr handle with GetStdHandle, redirects it to the log file with SetStdHandle, and restores it in a new #[cfg(windows)] Drop impl; a matching #[cfg(not(any(unix, windows)))] no-op Drop impl covers all remaining platforms.

Confidence Score: 5/5

Safe to merge; both changes are narrowly scoped and correct in their normal lifecycle.

The order-swap in ui.rs is a mechanical two-line move with no logical risk. The Windows redirect is structurally correct: the guard's custom Drop restores stderr before _file closes the handle, so the dual-reference to the same HANDLE is resolved in the right order during teardown.

The Windows redirect_stderr_to implementation in crates/tui/src/runtime_log.rs is the only spot worth a second look, specifically whether DuplicateHandle should be used instead of the raw handle from as_raw_handle().

Important Files Changed

Filename	Overview
crates/tui/src/runtime_log.rs	Adds Windows redirect_stderr_to via GetStdHandle/SetStdHandle with proper error propagation and a matching Drop impl; functionally correct for the normal lifecycle but shares the raw HANDLE between _file and the redirected stderr instead of using DuplicateHandle for true ownership isolation.
crates/tui/src/tui/ui.rs	Swaps EnterAlternateScreen to run after runtime_log::init(), so the stderr redirect is active before any alt-screen bytes are written; straightforward and correct.

Sequence Diagram

sequenceDiagram
    participant UI as run_tui()
    participant RL as runtime_log::init()
    participant OS as OS stderr
    participant Term as Terminal (alt-screen)

    Note over UI: enable_raw_mode()
    UI->>RL: init()
    RL->>OS: GetStdHandle / dup(STDERR_FILENO) — save original
    RL->>OS: SetStdHandle / dup2 — redirect stderr → log file
    RL-->>UI: TuiLogGuard (redirect active)
    UI->>Term: EnterAlternateScreen ✅ redirect already live
    Note over Term: TUI session — all stderr writes go to log file
    UI->>Term: LeaveAlternateScreen
    Note over UI: TuiLogGuard drops
    RL->>OS: SetStdHandle / dup2 — restore original stderr

Comments Outside Diff (2)

crates/tui/src/runtime_log.rs, line 2-4 (link)

The module-level docstring still says "(on Unix)" in the first paragraph and again in the "Defence-in-depth" bullet 2. Now that Windows has the same redirect, both spots should be updated to avoid misleading future contributors.
crates/tui/src/runtime_log.rs, line 53-55 (link)

The struct-level doc comment still reads "(on Unix) a saved copy of the original stderr fd", but the struct now also carries a Windows saved handle field. Worth updating alongside the module-level doc.

_{Reviews (3): Last reviewed commit: "docs(tui): update runtime log stderr red..." | Re-trigger Greptile}

…tderr redirect Two root causes of verbose logging leaking into the TUI alt-screen: 1. EnterAlternateScreen was called BEFORE runtime_log::init(), so any logging between alt-screen entry and redirect init leaked raw bytes into the TUI buffer — causing the 'scroll demon' on all platforms. 2. redirect_stderr_to was #[cfg(unix)] only — Windows had no stderr redirect at all, so every eprintln!/tracing call during the TUI session wrote directly into the alt-screen buffer. Fixes: - Swap order: init runtime_log (with stderr redirect) BEFORE entering the alt-screen, so the redirect is active from the start. - Add #[cfg(windows)] redirect_stderr_to using SetStdHandle from the already-available windows crate, with corresponding Drop impl to restore the original handle. Fixes #1909

gemini-code-assist

Code Review

This pull request introduces Windows support for stderr redirection in the TUI runtime log and reorders the initialization of the log guard to occur before entering the alternate screen, preventing rendering issues. The reviewer suggests checking the return value of SetStdHandle on Windows and propagating any errors instead of ignoring them.

gemini-code-assist · 2026-05-27T05:13:58Z

+    unsafe {
+        let _ = SetStdHandle(STD_ERROR_HANDLE, windows::Win32::Foundation::HANDLE(target as isize));
+    }


The return value of SetStdHandle is currently ignored. If SetStdHandle fails, the redirection will not take effect, but the function will still return Ok(saved). It is safer and more robust to check the return value of SetStdHandle and propagate any error so that the failure is properly handled and logged.

unsafe { SetStdHandle(STD_ERROR_HANDLE, windows::Win32::Foundation::HANDLE(target as isize)) .ok() .context("SetStdHandle(STD_ERROR_HANDLE) failed")?; }

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

devin-ai-integration

Devin Review found 1 potential issue.

View 3 additional findings in Devin Review.

devin-ai-integration · 2026-05-27T05:20:11Z

+    unsafe {
+        let _ = SetStdHandle(STD_ERROR_HANDLE, windows::Win32::Foundation::HANDLE(target as isize));
+    }


🟡 Windows redirect_stderr_to silently swallows SetStdHandle failure, returning false success

The Windows redirect_stderr_to function discards the SetStdHandle result with let _ = ... and unconditionally returns Ok(saved). If SetStdHandle fails, the caller (init() at crates/tui/src/runtime_log.rs:167) receives Ok(handle) from .ok(), stores Some(handle) in the guard, and believes stderr was redirected when it was not. On drop, the guard performs a pointless SetStdHandle to "restore" an already-current handle.

The Unix counterpart at crates/tui/src/runtime_log.rs:260-264 correctly checks the dup2 return value and propagates the error (also cleaning up the saved fd). The Windows version should do the same so that failure is visible and the guard stores None (via the .ok() in init) rather than a misleading Some.

Prompt for agents

In redirect_stderr_to (Windows), the SetStdHandle result is discarded with `let _ = ...` and the function returns Ok(saved) unconditionally. If SetStdHandle fails, this reports false success. The Unix counterpart properly checks dup2's return and returns Err on failure (also closing the saved fd). The fix should propagate the SetStdHandle error, e.g. by using `?` or `.context(...)?.` on the Result, so that the caller's `.ok()` in init() correctly produces None when the redirect failed. See the Unix error-handling pattern at lines 260-264 for reference.

Was this helpful? React with 👍 or 👎 to provide feedback.

Use .map_err() with a descriptive message and cast target to isize for the HANDLE constructor, so the error is properly propagated if SetStdHandle fails. Co-Authored-By: bot_apk <apk@cognition.ai>

greptile-apps

Hmbown has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.

greptile-apps

Hmbown has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.

Hmbown · 2026-05-27T12:35:55Z

Pushed af16bf9cb to repair the Devin follow-up. The previous head cast the Windows raw stderr handle to isize, but windows 0.60 defines HANDLE as HANDLE(*mut c_void), so Windows CI failed to compile. This commit passes the raw handle pointer directly and keeps rustfmt happy.

Local verification:

RUSTFLAGS=-Dwarnings CARGO_TARGET_DIR=/Volumes/VIXinSSD/whalebro/codewhale/target cargo check -p codewhale-tui --all-features --locked
cargo fmt --all -- --check
git diff --check

Hmbown · 2026-05-27T13:19:11Z

Independent review:

Approach is correct. Init-before-EnterAlternateScreen ordering does eliminate the leak window — any early tracing::error! from init() itself, or from log-dir resolution failures, now lands in the file instead of the alt-screen. Verified cargo check -p codewhale-tui clean on macOS; #[cfg(not(any(unix, windows)))] no-op Drop preserves wasm/redox builds.

Windows handle ownership (already greptile-flagged): as_raw_handle() aliases the HANDLE between _file and the stderr table. Normal drop order is safe (restore stderr → close file), but a third-party crate calling CloseHandle(GetStdHandle(STD_ERROR_HANDLE)) mid-session would double-close. DuplicateHandle before SetStdHandle would mirror the Unix dup isolation. Low risk in practice; worth a follow-up.

v0.8.48 / PR #2256: No conflict — #2256 touches runtime_log.rs only with a trivial clippy fix (.and_then(|h| resolve(h)) → .and_then(resolve)), well away from the redirect code.

Overlap with PR #2262: Same root issue (#1909), different mechanism — this PR redirects stderr fd/handle; #2262 suppresses the verbose CLI logger globally while alt-screen is active. They compose well (defense in depth: #2262 stops Codewhale's own verbose writes at the source, #2259 catches everything else — deps, panics, sub-agents) but produce a textual conflict in ui.rs around the EnterAlternateScreen block. Whichever lands second needs to keep the order: runtime_log::init() → EnterAlternateScreen → suppress_for_tui_alt_screen(). Recommend merging this first (smaller, lower-risk), then rebasing #2262 onto it.

…aliasing The previous implementation used file.as_raw_handle() directly with SetStdHandle, causing both _file and the process stderr table entry to share the same HANDLE. If a third-party library called CloseHandle on the stderr handle during the TUI session, it would silently invalidate _file and cause a double-CloseHandle on guard drop. Now we call DuplicateHandle before SetStdHandle to produce a truly independent handle for the redirected stderr, mirroring the Unix path's use of libc::dup. The duplicated handle is tracked in the guard struct and properly closed on drop after the original stderr is restored. Co-Authored-By: bot_apk <apk@cognition.ai>

greptile-apps

Hmbown has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.

greptile-apps

Hmbown has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.

Hmbown · 2026-05-31T07:49:51Z

Refreshed onto current main and preserved the runtime-log defense-in-depth path.

Merge-refresh details:

runtime_log::init() still runs before EnterAlternateScreen
Windows stderr redirect still uses DuplicateHandle
current main's Windows verbose-output suppression remains after alt-screen entry
after the merge, this PR only changes crates/tui/Cargo.toml, crates/tui/src/runtime_log.rs, and crates/tui/src/tui/ui.rs

Local verification:

cargo fmt --all -- --check
git diff --check
CARGO_TARGET_DIR=/Volumes/VIXinSSD/codewhale-pr2259-target cargo check -p codewhale-tui --all-features --locked
CARGO_TARGET_DIR=/Volumes/VIXinSSD/codewhale-pr2259-target cargo test -p codewhale-tui runtime_log --locked (5 runtime_log tests)
Windows API probe against windows = 0.60 for HANDLE / DuplicateHandle / SetStdHandle

A full local Windows target check was blocked by missing Windows C headers for aws-lc-sys, not by this PR. I am waiting for refreshed GitHub CI before merging.

greptile-apps

Hmbown has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.

Hmbown · 2026-05-31T08:02:56Z

Thanks again for this one. I merged current main into the branch after today’s PR harvest moved underneath it, resolved the single test-file conflict by keeping both the new spillover-root comment and the clippy allow, and pushed 5057eb78c back to the PR branch.

Local re-checks from the isolated worktree:

cargo fmt --all -- --check
git diff --cached --check
CARGO_TARGET_DIR=/Volumes/VIXinSSD/codewhale-pr2259-target cargo test -p codewhale-tui runtime_log --locked
CARGO_TARGET_DIR=/Volumes/VIXinSSD/codewhale-pr2259-target cargo check -p codewhale-tui --all-features --locked

CI should restart on the fresh head now; I’ll merge it as soon as the Windows lane comes back clean.

Copilot AI review requested due to automatic review settings May 27, 2026 05:12

Copilot started reviewing on behalf of Hmbown May 27, 2026 05:12 View session

gemini-code-assist Bot reviewed May 27, 2026

View reviewed changes

Copilot AI reviewed May 27, 2026

greptile-apps Bot reviewed May 27, 2026

View reviewed changes

Comment thread crates/tui/src/runtime_log.rs Outdated

devin-ai-integration Bot reviewed May 27, 2026

View reviewed changes

fix(tui): handle Windows stderr redirect API types

ec3380d

Hmbown mentioned this pull request May 27, 2026

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

Open

Hmbown and others added 2 commits May 27, 2026 06:22

docs(tui): update runtime log stderr redirect notes

fc7866d

fix: propagate SetStdHandle error instead of using .context()

56c3284

Use .map_err() with a descriptive message and cast target to isize for the HANDLE constructor, so the error is properly propagated if SetStdHandle fails. Co-Authored-By: bot_apk <apk@cognition.ai>

greptile-apps Bot reviewed May 27, 2026

View reviewed changes

fix(tui): pass raw Windows stderr handle

af16bf9

greptile-apps Bot reviewed May 27, 2026

View reviewed changes

Hmbown mentioned this pull request May 27, 2026

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

Closed

greptile-apps Bot reviewed May 28, 2026

View reviewed changes

Merge origin/main into fix/1909-windows-logging-alt-screen

30ae7dc

greptile-apps Bot reviewed May 31, 2026

View reviewed changes

Merge main into runtime log alt-screen fix

5057eb7

greptile-apps Bot reviewed May 31, 2026

View reviewed changes

Hmbown merged commit 28bd9b3 into main May 31, 2026
14 checks passed

Hmbown deleted the fix/1909-windows-logging-alt-screen branch May 31, 2026 08:12

Conversation

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

Uh oh!

Problem

Fix

Verification

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Comments Outside Diff (2)

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Hmbown commented May 27, 2026

Uh oh!

Hmbown commented May 27, 2026

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Hmbown commented May 31, 2026

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Hmbown commented May 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Hmbown commented May 27, 2026 •

edited by greptile-apps Bot

Loading