feat: ExternalTool wrappers for git, gh, rustc commands#2031
feat: ExternalTool wrappers for git, gh, rustc commands#2031aboimpinto wants to merge 21 commits into
Conversation
Introduces a central shell abstraction that replaces hardcoded
Command::new("cmd") / Command::new("sh") across the execution path.
- Shell detection at startup (pwsh -> powershell -> cmd -> sh)
- Correct quoting per shell (PowerShell uses -NoProfile -Command)
- Scope guards restore crossterm raw mode on all spawn paths (Hmbown#1690)
- Direct program+args builder for sandbox ExecEnv integration
Files:
- crates/tui/src/shell_dispatcher.rs (new, 12 tests)
- crates/tui/src/main.rs (register module)
- crates/tui/src/eval.rs (exec_shell delegates to dispatcher)
- crates/tui/src/sandbox/mod.rs (CommandSpec::shell uses dispatcher)
- crates/tui/src/tools/shell.rs (raw mode guards on all spawn paths)
Closes Hmbown#1690
Refs Hmbown#1779
… logging - find_exe(): fall back to known install dirs when PATH lookup fails (C:\Program Files\PowerShell\7, System32\WindowsPowerShell\v1.0) - Encoding prefix: use idiomatic [Console]::OutputEncoding for PowerShell instead of cmd.exe chcp 65001 >NUL - Execution logging: write exec via <ShellKind> entries to SHELL_DISPATCHER_LOG when set - System prompt: use ShellDispatcher detection instead of raw $SHELL so model knows it has PowerShell and generates native cmdlets - display_command(): strip PowerShell encoding prefix for display - Add tests for find_exe PATH + known-dir fallback Refs Hmbown#1779
- Fixed sandbox tests to accept sh/bash/zsh on non-Windows - Added cfg_shell_program() helper that uses ShellDispatcher detection - Marked find_exe_finds_cmd_on_path as #[cfg(windows)] - Fixed shell::tests for issue_1691 to accept sh/bash/zsh - Added #[cfg(windows)] to cmd-specific tests
ShellDispatcher may detect pwsh, powershell, or cmd on Windows. Tests now verify the command is passed as the last arg instead of hardcoding cmd.exe with chcp prefix.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a ShellDispatcher abstraction to centralize shell detection and command execution across different platforms and shell types (PowerShell, Cmd, Bash, etc.). It updates various components, including rustc_version detection, GitHub CLI interactions, and the sandbox environment, to utilize this new dispatcher. Feedback identifies critical issues with terminal state management where raw mode is unconditionally restored even if it wasn't initially active, potentially corrupting the terminal in non-TUI modes. Additionally, there is a concurrency concern regarding unhandled mutex poisoning in the logging logic which could lead to race conditions.
| // Disable raw mode before spawn; restore on drop regardless of | ||
| // success/failure/timeout (issue #1690). | ||
| let _ = crossterm::terminal::disable_raw_mode(); | ||
| struct SyncRawModeGuard; | ||
| impl Drop for SyncRawModeGuard { | ||
| fn drop(&mut self) { | ||
| let _ = crossterm::terminal::enable_raw_mode(); | ||
| } | ||
| } | ||
| let _guard = SyncRawModeGuard; |
There was a problem hiding this comment.
The SyncRawModeGuard unconditionally enables terminal raw mode when dropped. This causes a regression in non-TUI modes (such as codewhale exec), where raw mode is not initially active; the guard will leave the user's terminal in raw mode after the command finishes, breaking normal shell behavior (e.g., missing newlines, no echo). The guard should only restore raw mode if it was previously enabled, matching the pattern used in ShellDispatcher::run_foreground.
| // Disable raw mode before spawn; restore on drop (issue #1690). | ||
| let _ = crossterm::terminal::disable_raw_mode(); | ||
| struct InteractiveRawModeGuard; | ||
| impl Drop for InteractiveRawModeGuard { | ||
| fn drop(&mut self) { | ||
| let _ = crossterm::terminal::enable_raw_mode(); | ||
| } | ||
| } | ||
| let _guard = InteractiveRawModeGuard; |
There was a problem hiding this comment.
Similar to SyncRawModeGuard, InteractiveRawModeGuard unconditionally enables raw mode on drop. This will corrupt the terminal state if the manager is invoked from a context where raw mode was not already active (e.g., CLI mode). It should be updated to check the initial state and only restore if necessary.
| } | ||
|
|
||
| fn log_startup(kind: &ShellKind) { | ||
| let _lock = LOG_MUTEX.lock(); |
There was a problem hiding this comment.
The result of LOG_MUTEX.lock() is not handled for poisoning. If the mutex becomes poisoned (e.g., due to a panic while holding the lock), lock() returns an Err variant. Since the result is assigned to _lock without unwrapping or extracting the guard, the current thread will proceed into the critical section without actually holding the lock, leading to potential race conditions when writing to the log file. This pattern is repeated at lines 162 and 231.
1 participant
Stacked on ShellDispatcher (#2008). Replaces hardcoded Command::new calls with ExternalTool wrappers in main.rs.