Optionally run using user profile#1678
Conversation
bolinfest
left a comment
bolinfest
left a comment
There was a problem hiding this comment.
I think this is generally a good starting point, but my biggest question is whether we should scrap ShellEnvironmentPolicy and make this new behavior the default.
|
|
||
| #[cfg(target_os = "macos")] | ||
| pub fn current_shell() -> Option<Shell> { | ||
| let user = whoami::username(); |
There was a problem hiding this comment.
We should probably check $SHELL first and then do this as a fallback?
There was a problem hiding this comment.
From what I gathered $SHELL is a less reliable indicator of the default shell.
| Shell::Zsh(shell_path) => { | ||
| let mut result = vec![shell_path.clone(), "-c".to_string()]; | ||
| if let Ok(joined) = shlex::try_join(command.iter().map(|s| s.as_str())) { | ||
| result.push(format!("source ~/.zshrc && ({joined})")); |
There was a problem hiding this comment.
Is ~/.zshrc required to exist? Should ; be used instead of && to ignore a failure in that case?
There was a problem hiding this comment.
A well-behaved ~/.zshrc should not do this, but it could write to stdout/stderr and interfere with the tool call output, no?
There was a problem hiding this comment.
Out of curiosity, are the parens around necessary?
Though I did try the following and it still exits 42, at least:
ls && (python -c 'import sys; sys.exit(42)')
There was a problem hiding this comment.
Out of curiosity, are the parens around necessary?
in case there are more && || in the command we are trying to run
There was a problem hiding this comment.
Added logic to check for zshrc
| pub include_only: Vec<EnvironmentVariablePattern>, | ||
|
|
||
| /// If true, the shell profile will be used to run the command. | ||
| pub use_profile: bool, |
There was a problem hiding this comment.
Currently, I am tempted to get rid of ShellEnvironmentPolicy altogether and make the user responsible for sanitizing the environment with which they invoke codex. Or at least, make the default behavior to inherit the current environment and the user has to take action to configure something else.
Thoughts?
There was a problem hiding this comment.
I don't mind it but I'd like to avoid bundling everything together.
| stdio_policy: StdioPolicy, | ||
| env: HashMap<String, String>, | ||
| ) -> std::io::Result<Child> { | ||
| trace!( |
There was a problem hiding this comment.
I left this to be able to ask for logs from users when shell exec does something strange
| uuid = { version = "1", features = ["serde", "v4"] } | ||
| wildmatch = "2.4.0" | ||
| whoami = "1.6.0" | ||
| shlex = "1.3.0" |
| let home = std::env::var("HOME").unwrap(); | ||
| let shell_path = String::from_utf8_lossy(&shell.stdout).trim().to_string(); | ||
| if shell_path.ends_with("/zsh") { | ||
| assert_eq!( |
There was a problem hiding this comment.
I guess it is fair to assume this test should pass on an arbitrary Mac?
There was a problem hiding this comment.
Yes, should pass (and does in ci)
| } | ||
|
|
||
| impl Shell { | ||
| pub fn run_with_profile(&self, command: Vec<String>) -> Option<Vec<String>> { |
There was a problem hiding this comment.
This doesn't actually "run" anything: it just rewrites the command? Maybe create_command_shell_invocation()? I don't know, naming is hard...
2 participants
No description provided.