Conversation
bolinfest
changed the title
bolinfest
changed the title
bolinfest
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a73d681558
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let updated_path_env_var = match std::env::var_os("PATH") { | ||
| Some(existing_path) => { | ||
| let mut path_env_var = | ||
| std::ffi::OsString::with_capacity(path.as_os_str().len() + 1 + existing_path.len()); | ||
| path_env_var.push(path); | ||
| path_env_var.push(PATH_SEPARATOR); | ||
| path_env_var.push(existing_path); | ||
| path_env_var | ||
| } | ||
| None => path.as_os_str().to_owned(), |
There was a problem hiding this comment.
On Unix, var_os("PATH") can return raw bytes, so this now preserves a non-Unicode PATH in the current process. Startup still succeeds, but later code paths still assume the environment is UTF-8: core/src/exec_env.rs:24 (used by the shell tool in core/src/tools/handlers/shell.rs:75 and unified exec in core/src/unified_exec/process_manager.rs:586) and shell-escalation/src/unix/escalate_client.rs:48 both iterate with std::env::vars(), which panics when PATH is not Unicode. Before this change the Err(NotUnicode) branch replaced PATH with the helper dir's UTF-8 string, so those flows kept working; with this version, users who launch Codex from a shell with a non-UTF8 path entry can crash as soon as they run a shell or escalated command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I'll address this in a follow-up.
celia-oai
left a comment
celia-oai
left a comment
There was a problem hiding this comment.
might be worth adding a unit test?
I'll do that as part of the PR stacked on top! |
2 participants
Why
prepend_path_entry_for_codex_aliases()was rebuildingPATHthroughstd::env::var("PATH")andStringformatting. That assumes the inheritedPATHis valid UTF-8, which is not guaranteed on supported platforms. WhenPATHcontains non-UTF-8 bytes, Codex can fail to prepend the per-session helper directory even though the existing environment is otherwise usable.What changed
PATHwithstd::env::var_os()instead ofstd::env::var()OsStringinstead of a formattedStringPATHcontents while still prepending the arg0 helper directoryHow to verify
PATHis reconstructed beforestd::env::set_var().Stack created with Sapling. Best reviewed with ReviewStack.