feat: use the arg0 trick with apply_patch #2646
Conversation
bolinfest
force-pushed
the
pr2646
branch
2 times, most recently
from
6f18d46 to
e962e55
Compare
bolinfest
force-pushed
the
pr2646
branch
2 times, most recently
from
ffd991b to
d10b861
Compare
dylan-hurd-oai
left a comment
dylan-hurd-oai
left a comment
There was a problem hiding this comment.
2 discussions about edge cases, but LGTM!
|
|
||
| // Retain the TempDir so it exists for the lifetime of the invocation of | ||
| // this executable. Admittedly, we could invoke `keep()` on it, but it | ||
| // would be nice to avoid leaving temporary directories behind, if possible. |
There was a problem hiding this comment.
+1, i think this is a reasonable approach
| }; | ||
|
|
||
| unsafe { | ||
| std::env::set_var("PATH", updated_path_env_var); |
There was a problem hiding this comment.
Thinking out loud: set_var can technically panic if updated_path_env_var contains a null char, but I think we're reasonably safe here since std::env::var would return VarError in that case, and temp_dir.path() should not return a null char, right?
There was a problem hiding this comment.
On both Windows and UNIX, a file path can never contain NUL, so correct, we don't have to worry about that here.
codex-rs/arg0/src/lib.rs
Outdated
| // While it is possible that Codex could likely proceed successfully | ||
| // even if updating the PATH fails, let's be strict. | ||
| eprintln!("could not update PATH: {err}"); | ||
| std::process::exit(1); |
There was a problem hiding this comment.
I actually think we may want to proceed here? It feels like this should be rare, but we could still proceed optimistically. Or log a more helpful error here for the user to know what happened and why we're exiting.
There was a problem hiding this comment.
I suppose we can eprintln! since there is no logging library set up at this point...
There was a problem hiding this comment.
OK, removed the exit() call here and updated comment.
Historically, Codex CLI has treated
apply_patch(and its sometimes misspelling,applypatch) as a "virtual CLI," intercepting it when it appears as the first arg tocommandfor the"container.exec","shell", or"local_shell"` tools.This approach has a known limitation where if, say, the model created a Python script that runs
apply_patchand then tried to run the Python script, we have no insight as to what the model is trying to do and the Python Script would fail becauseapply_patchwas never really on thePATH.One way to solve this problem is to require users to install an
apply_patchexecutable alongside thecodexexecutable (or at least put it someplace where Codex can discover it). Though to keep Codex CLI as a standalone executable, we exploit "the arg0 trick" where we create a temporary directory with an entry namedapply_patchand prepend that directory to thePATHfor the duration of the invocation of Codex.apply_patchis a symlink tocodex, which now changes its behavior to behave likeapply_patchif arg0 isapply_patch(orapplypatch)apply_patch.batis a batch script that runscodex --codex-run-as-apply-patch %*, as Codex also changes its behavior if the first argument is--codex-run-as-apply-patch.