fix: open scrollback editor on windows without sh

refs #914
This commit is contained in:
Ogulcan Celik 2026-07-02 01:16:21 +03:00
parent 6a5b431e39
commit a9111cbab2
7 changed files with 174 additions and 26 deletions

View File

@ -113,6 +113,7 @@ external contributor guardrail.
### Windows VM validation
The Windows VM is for final/manual Windows validation, not normal agent work.
Connect to it with the `windows-wirt` SSH alias.
Use the single reusable checkout at `C:\work\repo`. Do not create additional
persistent Herdr clones or worktrees on the VM. The Windows account is already
@ -121,11 +122,15 @@ named `herdr`, so avoid paths like `C:\Users\herdr\herdr`.
Before validating a fix on Windows, sync or apply the Linux worktree changes
into `C:\work\repo`, then run the needed Windows build or test commands there.
Reuse the shared Rust caches under `C:\Users\herdr\.cargo` and
`C:\Users\herdr\.rustup`. Do not use WSL on the VM.
`C:\Users\herdr\.rustup`. Do not use WSL on the VM. The VM may have a newer
Zig on `PATH`; Herdr currently requires Zig 0.15.2, so set
`$env:ZIG = "C:\Users\herdr\zig-0.15.2\zig.exe"` before running Cargo commands
that build the vendored libghostty-vt.
After validation, leave `C:\work\repo` clean. Remove temporary files and delete
`C:\work\repo\target` when disk space is tight, but keep the shared Cargo and
Rustup caches.
Rustup caches. Unless Can explicitly asks to keep the patched tree for more
manual testing, reset `C:\work\repo` back to a clean checkout before finishing.
## Agent Detection Updates

View File

@ -15,6 +15,7 @@
- Bumped the client/server protocol version to 15 for socket API placement mutation event and response compatibility.
### Fixed
- `prefix+e` scrollback editor panes now open on Windows without trying to run `/bin/sh`; Windows uses `VISUAL`, then `EDITOR`, then `notepad.exe` as the fallback editor. (#914)
- `herdr pane split --current` now resolves to the calling Herdr pane instead of the UI-focused pane when run inside a pane. (#902)
- Native Windows clients running inside Alacritty now preserve mouse reports and `ctrl+j` input instead of leaking mouse escape sequences into panes. `shift+enter` remains dependent on whether the outer terminal reports it as a distinct modified Enter key. (#792)
- OMP integration state now recovers after resumed sessions such as `omp -c` and reports Ask/tool approval waits as blocked instead of leaving the pane working or stuck on the previous OMP session. (#879)

View File

@ -820,14 +820,27 @@ impl App {
let path = write_scrollback_temp_file(&scrollback)?;
let quoted_path = shell_quote(&path.display().to_string());
let command = format!(
r#"scrollback_file={quoted_path}; eval "${{EDITOR:-vi}} \"\$scrollback_file\""; status=$?; rm -f "$scrollback_file"; exit $status"#
);
if let Err(err) = self.spawn_pane_command(&command, vec![path.clone()]) {
let _ = fs::remove_file(&path);
return Err(err);
}
let argv = match crate::platform::scrollback_editor_argv(&path) {
Ok(argv) => argv,
Err(err) => {
let _ = fs::remove_file(&path);
return Err(err);
}
};
let (env, _) = self.custom_command_env();
let new_pane = match self.spawn_overlay_argv_command(&argv, None, env, vec![path.clone()]) {
Ok((_, new_pane)) => new_pane,
Err(err) => {
let _ = fs::remove_file(&path);
return Err(err);
}
};
let terminal_id = new_pane.terminal.id.clone();
self.terminal_runtimes
.insert(terminal_id.clone(), new_pane.runtime);
self.state
.remove_alias_shadowed_by_new_pane(new_pane.pane_id);
self.state.terminals.insert(terminal_id, new_pane.terminal);
if let Some(public_pane_id) = self.public_pane_id(ws_idx, pane_id) {
self.state.toast = Some(crate::app::state::ToastNotification {
@ -1677,22 +1690,6 @@ fn unique_scrollback_path(attempt: u32) -> std::path::PathBuf {
))
}
fn shell_quote(value: &str) -> String {
if !value.is_empty()
&& value.chars().all(|ch| {
ch.is_ascii_alphanumeric()
|| matches!(
ch,
'@' | '%' | '_' | '+' | '=' | ':' | ',' | '.' | '/' | '-'
)
})
{
return value.to_string();
}
format!("'{}'", value.replace('\'', "'\\''"))
}
#[cfg(test)]
mod tests {
#[cfg(unix)]
@ -2797,6 +2794,13 @@ last_pane = "prefix+tab"
assert!(content.contains("alpha"));
assert!(content.contains("beta"));
assert_eq!(app.state.mode, Mode::Terminal);
assert!(
app.state.terminals.values().any(|terminal| terminal
.launch_argv
.as_ref()
.is_some_and(|argv| argv.first().is_some_and(|program| program == "/bin/sh"))),
"scrollback editor should launch through argv overlay path"
);
let _ = std::fs::remove_file(output_path);
}

View File

@ -6,6 +6,14 @@ use super::{ClipboardImage, ForegroundJob, Signal};
/// Unsupported platform stub.
pub fn raise_server_nofile_limit() {}
/// Unsupported platform stub.
pub(crate) fn scrollback_editor_argv(_path: &std::path::Path) -> std::io::Result<Vec<String>> {
Err(std::io::Error::new(
std::io::ErrorKind::Unsupported,
"opening scrollback in an editor is not supported on this platform",
))
}
/// Unsupported platform stub.
pub fn detach_server_daemon_command(_command: &mut Command) {}

View File

@ -12,6 +12,30 @@ use super::{
pub fn raise_server_nofile_limit() {}
pub(crate) fn scrollback_editor_argv(path: &std::path::Path) -> std::io::Result<Vec<String>> {
let quoted_path = shell_quote(&path.display().to_string());
let command = format!(
r#"scrollback_file={quoted_path}; eval "${{EDITOR:-vi}} \"\$scrollback_file\""; status=$?; rm -f "$scrollback_file"; exit $status"#
);
Ok(vec!["/bin/sh".to_string(), "-c".to_string(), command])
}
fn shell_quote(value: &str) -> String {
if !value.is_empty()
&& value.chars().all(|ch| {
ch.is_ascii_alphanumeric()
|| matches!(
ch,
'@' | '%' | '_' | '+' | '=' | ':' | ',' | '.' | '/' | '-'
)
})
{
return value.to_string();
}
format!("'{}'", value.replace('\'', "'\\''"))
}
/// Collect the foreground terminal job for a given child PID.
pub fn foreground_job(child_pid: u32) -> Option<ForegroundJob> {
let tpgid = foreground_process_group_id(child_pid)?;
@ -794,4 +818,15 @@ mod tests {
let _ = std::fs::remove_file(&path);
assert_eq!(args, "--\n-danger\nbody\n");
}
#[test]
fn scrollback_editor_argv_preserves_unix_editor_shell_semantics() {
let path = std::path::Path::new("/tmp/herdr scrollback.txt");
let argv = scrollback_editor_argv(path).unwrap();
assert_eq!(argv[0], "/bin/sh");
assert_eq!(argv[1], "-c");
assert!(argv[2].contains("EDITOR:-vi"));
assert!(argv[2].contains("/tmp/herdr scrollback.txt"));
}
}

View File

@ -16,6 +16,30 @@ const PROC_PGRP_ONLY: u32 = 2;
const SERVER_NOFILE_LIMIT_TARGET: libc::rlim_t = 8192;
const CF_STRING_ENCODING_UTF8: u32 = 0x0800_0100;
pub(crate) fn scrollback_editor_argv(path: &Path) -> std::io::Result<Vec<String>> {
let quoted_path = shell_quote(&path.display().to_string());
let command = format!(
r#"scrollback_file={quoted_path}; eval "${{EDITOR:-vi}} \"\$scrollback_file\""; status=$?; rm -f "$scrollback_file"; exit $status"#
);
Ok(vec!["/bin/sh".to_string(), "-c".to_string(), command])
}
fn shell_quote(value: &str) -> String {
if !value.is_empty()
&& value.chars().all(|ch| {
ch.is_ascii_alphanumeric()
|| matches!(
ch,
'@' | '%' | '_' | '+' | '=' | ':' | ',' | '.' | '/' | '-'
)
})
{
return value.to_string();
}
format!("'{}'", value.replace('\'', "'\\''"))
}
#[repr(C)]
struct TisInputSource {
_private: [u8; 0],
@ -1117,4 +1141,15 @@ printf '%s\n' "$@" > "$HERDR_NOTIFY_ARGS"
"-e\non run argv\n-e\ndisplay notification (item 2 of argv) with title (item 1 of argv)\n-e\nend run\ntitle\nbody\n"
);
}
#[test]
fn scrollback_editor_argv_preserves_unix_editor_shell_semantics() {
let path = std::path::Path::new("/tmp/herdr scrollback.txt");
let argv = scrollback_editor_argv(path).unwrap();
assert_eq!(argv[0], "/bin/sh");
assert_eq!(argv[1], "-c");
assert!(argv[2].contains("EDITOR:-vi"));
assert!(argv[2].contains("/tmp/herdr scrollback.txt"));
}
}

View File

@ -46,6 +46,41 @@ struct WindowsProcessEntry {
pub fn raise_server_nofile_limit() {}
pub(crate) fn scrollback_editor_argv(path: &std::path::Path) -> std::io::Result<Vec<String>> {
let editor = std::env::var("VISUAL")
.ok()
.filter(|value| !value.trim().is_empty())
.or_else(|| {
std::env::var("EDITOR")
.ok()
.filter(|value| !value.trim().is_empty())
});
scrollback_editor_argv_with_env(path, editor.as_deref())
}
fn scrollback_editor_argv_with_env(
path: &std::path::Path,
editor: Option<&str>,
) -> std::io::Result<Vec<String>> {
let mut argv = match editor.filter(|value| !value.trim().is_empty()) {
Some(editor) => command_line_to_argv(editor).ok_or_else(|| {
std::io::Error::new(
std::io::ErrorKind::InvalidInput,
format!("failed to parse editor command {editor:?}"),
)
})?,
None => vec!["notepad.exe".to_string()],
};
if argv.is_empty() {
return Err(std::io::Error::new(
std::io::ErrorKind::InvalidInput,
"editor command must not be empty",
));
}
argv.push(path.display().to_string());
Ok(argv)
}
pub fn detach_server_daemon_command(_command: &mut std::process::Command) {}
pub fn current_process_is_detached_server_daemon() -> bool {
@ -698,6 +733,31 @@ mod tests {
assert_eq!(pids, vec![10, 20, 30]);
}
#[test]
fn scrollback_editor_argv_uses_editor_env_and_appends_path() {
let path = std::path::Path::new(r"C:\Users\User\AppData\Local\Temp\herdr scrollback.txt");
let argv = super::scrollback_editor_argv_with_env(
path,
Some(r#""C:\Program Files\Microsoft VS Code\Code.exe" --wait"#),
)
.unwrap();
assert_eq!(argv[0], r"C:\Program Files\Microsoft VS Code\Code.exe");
assert_eq!(argv[1], "--wait");
assert_eq!(argv[2], path.display().to_string());
}
#[test]
fn scrollback_editor_argv_falls_back_to_notepad() {
let path = std::path::Path::new(r"C:\Temp\herdr-scrollback.txt");
let argv = super::scrollback_editor_argv_with_env(path, None).unwrap();
assert_eq!(
argv,
vec!["notepad.exe".to_string(), path.display().to_string()]
);
}
fn test_entry(
pid: u32,
parent_pid: u32,