Platform-aware default shell + Windows recording input fixes#199
Merged
Conversation
jrozner
force-pushed
the
worktree-default-shell
branch
from
10d5970 to
73d93dc
Compare
The Windows recorder needs a handful of console FFI calls (VT mode setup and waiting on the console input handle) that have no safe-wrapper equivalent the way `rustix` covers the Unix syscalls. `#![forbid(unsafe_code)]` cannot be overridden locally, so relax it to `#![deny(unsafe_code)]`: the no-unsafe default still holds crate-wide, but a module can opt in with a scoped, documented `#[allow(unsafe_code)]`. The Windows console module is the sole opt-in.
jrozner
force-pushed
the
worktree-default-shell
branch
from
73d93dc to
45ccc26
Compare
`recorder::unix::default_shell` and `recorder::windows::default_shell` were public but unused: the recorder always receives an explicit shell from the menu, and the platform-appropriate default now lives in `config::default_shell`. Drop them so there is a single source of truth for the default shell.
Default recording shell is now platform-aware (config::default_shell), used by
both Config::with_defaults() and the menu's runtime fallback:
* Unix: $SHELL, falling back to /bin/sh.
* Windows: pwsh.exe when PowerShell 7 is on PATH, else powershell.exe.
Previously with_defaults() read $SHELL on every platform, so Windows got an
empty default and fell back to /bin/sh, which does not exist there.
pwsh is preferred because aterm forwards its environment to the child: launched
from pwsh, the inherited PSModulePath points at PowerShell 7's modules, and
handing that to a Windows PowerShell 5.1 child makes it load PS7 module versions
incompatible with 5.1 — PSReadLine and even core modules fail to load ("Cannot
load PSReadline module"). Launching pwsh keeps the shell and its PSModulePath
consistent.
Windows console handling for the recording (no-op on Unix):
* TerminalModeGuard enables ENABLE_VIRTUAL_TERMINAL_INPUT (and clears the
cooked-input flags) so special keys — arrows, Home/End, function keys — are
captured as VT escape sequences and forwarded to the child; ReadConsoleW
otherwise returns only typed characters and drops them. It also enables
ENABLE_VIRTUAL_TERMINAL_PROCESSING + DISABLE_NEWLINE_AUTO_RETURN so the
child's VT output (colours, cursor motion) renders instead of printing escape
bytes. The exact prior console modes are restored on drop.
* StdinPoller's reader thread gates each blocking console read behind a 50ms
WaitForSingleObject on the console input handle, so it re-checks its stop
flag and exits on its own at teardown. This keeps the thread from outliving
the recording and stealing the post-recording menu's first keystroke, and
avoids an un-interruptible read that would otherwise hang the menu until a
keypress.
The Windows console FFI opts into unsafe via a scoped, documented
#[allow(unsafe_code)] on recorder::windows (the crate is otherwise
deny(unsafe_code)); each call has a SAFETY comment. Adds windows-sys as a
cfg(windows) dependency.
Verified: 164 tests pass; clippy clean on the native and x86_64-pc-windows-gnu
targets. Windows runtime behavior confirmed by the reporter (PSReadLine loads,
special keys work, exit drops straight to the menu).
jrozner
force-pushed
the
worktree-default-shell
branch
from
45ccc26 to
76060c7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
$SHELL(→/bin/sh) on Unix, PowerShell on Windows. PreviouslyConfig::with_defaults()read$SHELLon every platform, so on Windows it seeded an empty shell and fell back to/bin/sh, which doesn't exist there. Logic is centralized inconfig::default_shell()and shared bywith_defaults()and the menu's runtime fallback; the unused per-platformdefault_shell()helpers in the recorder are removed.ENABLE_VIRTUAL_TERMINAL_INPUT— so arrow keys / Ctrl-R were never encoded as the VT sequences PowerShell + PSReadLine need. NewTerminalModeGuardenables VT input + VT output processing for the recording and restores the exact prior console modes on drop.StdinPollerspawned a thread blocked in a console read and never stopped it, so after recording it stole the post-recording menu's first keystroke. It now cancels the blocking read (CancelSynchronousIo) and joins on drop.forbid(unsafe_code)→deny(unsafe_code). The Windows console APIs have no safe-wrapper equivalent (the wayrustixcovers the Unix syscalls), so the crate-wide invariant is relaxed todenywith a single scoped, documented#[allow(unsafe_code)]onrecorder::windowsalone. Each FFI call has aSAFETYcomment.TERM/SHELLpass through when set and are absent otherwise (native Windows defines neither).Testing
cargo test— 164 pass.cargo clippy --all-targetsandcargo clippy --target x86_64-pc-windows-gnu— clean.CancelSynchronousIoteardown timing) still needs on-device validation — Windows can't be run in this environment.Notes