fix(pty-proxy): resolve SHELL path in proxied zsh sessions#3607
fix(pty-proxy): resolve SHELL path in proxied zsh sessions#3607hangrymuppet wants to merge 1 commit into
Conversation
Fossier: Manual Review Requested
Score BreakdownTotal Score: 58.2/100 | Confidence: 74% | Outcome: REVIEW
|
Greptile SummaryThis PR changes how proxied zsh sessions choose the shell path. The main changes are:
Confidence Score: 4/5The changed flow looks mergeable after tightening the zsh path selection.
crates/atuin-pty-proxy/src/pty_proxy.rs Important Files Changed
Reviews (1): Last reviewed commit: "fix(pty-proxy): resolve SHELL path in pr..." | Re-trigger Greptile |
2c7fc8b to
520ceea
Compare
- Add target-specific parent process executable path resolution (via
proc_pidpath on macOS and /proc on Linux/Solaris/Illumos).
- Dynamically embed the resolved absolute path of the running zsh
binary in the generated init script.
- Resolve relative shell names to absolute paths in Rust before
spawning, ensuring $SHELL is populated correctly.
- Add a unit test to verify target-specific process path resolution
logic on the active OS.
Fixes atuinsh#3606
Link: atuinsh#3606
520ceea to
8bd8ab5
Compare
| let shell_path = if let Some(ref path) = options.shell { | ||
| if path.is_absolute() { | ||
| Some(path.clone()) | ||
| } else if let Ok(paths) = std::env::var("PATH") { | ||
| // Resolve via $PATH | ||
| std::env::split_paths(&paths) | ||
| .map(|p| p.join(path)) | ||
| .find(|p| p.is_file()) | ||
| // Fallback if not found in $PATH | ||
| .or_else(|| Some(path.clone())) | ||
| } else { | ||
| // No $PATH variable | ||
| Some(path.clone()) | ||
| } | ||
| } else { | ||
| // No shell specified | ||
| None | ||
| }; |
There was a problem hiding this comment.
| let shell_path = if let Some(ref path) = options.shell { | |
| if path.is_absolute() { | |
| Some(path.clone()) | |
| } else if let Ok(paths) = std::env::var("PATH") { | |
| // Resolve via $PATH | |
| std::env::split_paths(&paths) | |
| .map(|p| p.join(path)) | |
| .find(|p| p.is_file()) | |
| // Fallback if not found in $PATH | |
| .or_else(|| Some(path.clone())) | |
| } else { | |
| // No $PATH variable | |
| Some(path.clone()) | |
| } | |
| } else { | |
| // No shell specified | |
| None | |
| }; | |
| let shell_path = match options.shell { | |
| Some(ref path) => if path.is_absolute() { | |
| Some(path.clone()) | |
| } else if let Ok(paths) = std::env::var("PATH") { | |
| // Resolve via $PATH | |
| std::env::split_paths(&paths) | |
| .map(|p| p.join(path)) | |
| .find(|p| p.is_file()) | |
| // Fallback if not found in $PATH | |
| .or_else(|| Some(path.clone())) | |
| } else { | |
| // No $PATH variable | |
| Some(path.clone()) | |
| } | |
| // No shell specified | |
| None => None, | |
| }; |
Or even use options.shell.as_ref().map(|path| { /* ... */ }) maybe ?
| fn get_parent_shell_path(target_shell: Shell) -> Option<String> { | ||
| #[cfg(unix)] | ||
| { | ||
| let ppid = unsafe { libc::getppid() }; |
There was a problem hiding this comment.
| let ppid = unsafe { libc::getppid() }; | |
| let ppid = std::os::unix::process::parent_id(); |
There was a problem hiding this comment.
With this change it's also possible to make the libc dep macos-only, see https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies
| #[cfg(target_os = "macos")] | ||
| let path = { | ||
| let mut buf = [0u8; 4096]; | ||
| let res = unsafe { |
There was a problem hiding this comment.
Could you add // Safety: comments on every unsafe block ?
| if let Some(path) = path | ||
| && let Some(detected_shell) = shell_from_name(&path.to_string_lossy()) | ||
| && detected_shell == target_shell | ||
| { | ||
| return Some(path.to_string_lossy().into_owned()); | ||
| } |
There was a problem hiding this comment.
| if let Some(path) = path | |
| && let Some(detected_shell) = shell_from_name(&path.to_string_lossy()) | |
| && detected_shell == target_shell | |
| { | |
| return Some(path.to_string_lossy().into_owned()); | |
| } | |
| if let Some(path) = path | |
| && let path = path.to_string_lossy() | |
| && let Some(detected_shell) = shell_from_name(&path) | |
| && detected_shell == target_shell | |
| { | |
| return Some(path.into_owned()); | |
| } |
Avoids doing the to_string_lossy() twice (which does a UTF-8 check each time)
Fixes self-reported #3606; regression introduced after #3548
Thank you for maintaining this awesome piece of work!
Checks