fix: harden arg0 helper PATH handling #8766
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
viyatb-oai
changed the title
viyatb-oai
changed the title
|
I have read the CLA Document and I hereby sign the CLA |
![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: 062b2cdc8b
ℹ️ 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".
|
It looks like |
codex-rs/arg0/src/lib.rs
Outdated
| let temp_dir = tempfile::Builder::new() | ||
| .prefix("codex-arg0") | ||
| .tempdir_in(codex_home)?; |
There was a problem hiding this comment.
I worry that this may make a bit of a mess of things in $CODEX_HOME if codex crashes and these temp dirs do not get cleaned up.
I feel like we should create $CODEX_HOME/tmp (or maybe some other name?) first and then use that as the argument to tempdir_in().
While this specific TempDir is designed to be a $PATH entry, I have wondered whether we should also create an entry under $CODEX_HOME to use when $TMP_DIR is not defined (which is often the case on Linux) so that it is preferred over /tmp, though I'm not sure how many libraries would actually honor that.
Anyway, the point being, if we introduced $CODEX_HOME/tmp, perhaps we would want different subfolders for different purposes, so we might want this to be $CODEX_HOME/tmp/path, for example.
What do you think?
Also, should we chmod this folder at all?
There was a problem hiding this comment.
I worry that this may make a bit of a mess of things in $CODEX_HOME if codex crashes and these temp dirs do not get cleaned up.
I agree if codex crashes it might mess up cleaning of the temp directories.
While this specific TempDir is designed to be a $PATH entry, I have wondered whether we should also create an entry under $CODEX_HOME to use when $TMP_DIR is not defined (which is often the case on Linux) so that it is preferred over /tmp, though I'm not sure how many libraries would actually honor that.
true, I forgot about the linux usecase here.
Also, should we chmod this folder at all?
good catch, yes!
cargo fmt fix
Motivation
CODEX_HOMEinstead ofstd::env::temp_dir().CODEX_HOMEvalues that live under the system temp root to prevent writable PATH entries.Testing
just fmt, which completed with a non-blockingimports_granularitywarning.just fix -p codex-arg0(Clippy fixes) which completed successfully.cargo test -p codex-arg0and the test run completed successfully.