Conversation
Performance Benchmark Report
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is Test Summary: bash-completion test suite
|
reubeno
left a comment
reubeno
left a comment
There was a problem hiding this comment.
Thanks for putting this together! I've got a few questions/requests. My biggest question is what the actual call stack of the failing API looks like so I can better understand whether this is the best place for the check.
|
|
||
| #[test] | ||
| fn test_setup_terminal_settings_skips_file_input() { | ||
| let mut tmp = std::env::temp_dir(); |
There was a problem hiding this comment.
Instead of manually creating a file in the temp dir and cleaning it up, could you please use tempfile::tempfile? We use that crate elsewhere in testing to simplify auto-cleanup.
|
|
||
| use super::*; | ||
|
|
||
| // ==================== setup_terminal_settings tests ==================== |
| // Terminal settings only make sense for terminal input. `AutoModeGuard::new` | ||
| // keys off the process's standard handles, which may be a console even when | ||
| // the read input is a pipe/file/redirect (e.g. an agent shell hosted in a | ||
| // terminal): in that case `apply_settings` would hit `SetConsoleMode` on the |
There was a problem hiding this comment.
Can you clarify where the call to SetConsoleMode happens -- or what the caller is? I don't see any direct calls to it in brush, nor do we have any Windows-specific console paths right now. (Perhaps I've overlooked something, though?)
| &self, | ||
| file: &brush_core::openfiles::OpenFile, | ||
| ) -> Result<Option<brush_core::terminal::AutoModeGuard>, brush_core::Error> { | ||
| // Terminal settings only make sense for terminal input. `AutoModeGuard::new` |
There was a problem hiding this comment.
Meta-note: this comment is Windows-centric; could you simplify it and clarify which parts are platform specific?
2 participants
Problem
On Windows,
readfails withos error 87(ERROR_INVALID_PARAMETER) whenever the input is a pipe, file, or redirect AND the host process's standard handles are a console. This is the common case for an agent shell hosted in a terminal (e.g. a coding agent runningprintf 'x\n' | while read -r line; ...), where the shell runs in-process in the TUI host.Root cause
setup_terminal_settingsalways callsAutoModeGuard::new(file)and applies settings.AutoModeGuard::new'sfrom_termkeys off the process's standard handles (GetStdHandle) rather than thefileargument — so in a terminal-hosted process it succeeds even when the read input is a pipe/file/redirect.apply_settingsthen callsSetConsoleModeon the non-console input, which fails with ERROR_INVALID_PARAMETER, and the?propagates the failure, failing the whole read.Subprocesses never hit this because their stdin is a pipe, so
AutoModeGuard::newfails and the.ok()degrades to no-op.Fix
Skip terminal-mode setup entirely for non-terminal input (
if !file.is_terminal() { return Ok(None); }), matching bash's termios behavior (bash only applies terminal settings to terminal input).Tests
Two contract tests: pipe input and file input both return
Ok(None)(no mode guard installed, no error).Verified on Windows: after this change
printf 'ok\n' | while IFS= read -r x; do echo "$x"; doneworks in the terminal-hosted agent shell.