mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
unified-exec: preserve PathUri through exec-server (#28681)
## Why It should be possible for app-server to handle "foreign" OS paths in unified_exec working directories, allowing e.g. a Linux app-server to run processes on e.g. a Windows exec-server. ## What Convert the core unified_exec cwd values to use `PathUri`. Adds fallible path conversion in several places to try to minimize the scope of this change. The only time this change suppresses errors from converting `PathUri` to an `AbsolutePathBuf` is when the turn is configured with no sandboxing at all to allow us to make progress testing without sandboxing. Future changes to apply_patch and sandboxing will clean up these error paths. A tool's cwd is resolved from joining a model-provided workdir to the environment's cwd. When using `AbsolutePathBuf::join()`, an absolute-path workdir would overwrite the environment's cwd and we would resolve permissions/sandboxing against the model-provided path. This change extends `PathUri::join()` to also treat an absolute rhs as an override of the base/lhs. This also removes some coverage from the remove_env_windows tests until a follow-up converts foreign paths in command exec events correctly. ## Breaking Changes When using `AbsolutePathBuf::join()` for workdir resolution, we ended up resolving tilde-prefixed paths against the app-server's `$HOME`, e.g. `~/foo/bar` becomes `/home/anp/foo/bar`. It's difficult to do this with `PathUri` joining, so after offline discussion this PR no longer implements it. A quick check of some power users' rollouts suggests that models don't actually generate home-prefixed absolute working directories for their spawns, so this shouldn't have any real blast radius.
This commit is contained in:
1 parent
cca39d51ba
commit
5867b529ae
33 files changed
+634
-206
No files matched your search
@@ -211,18 +211,20 @@ impl PathUri {
|
||||
Some(Self(url))
|
||||
}
|
||||
|
||||
/// Lexically joins a relative URI path onto this URI.
|
||||
/// Lexically resolves native absolute or relative path text against this URI.
|
||||
///
|
||||
/// Path text is interpreted using the POSIX or Windows convention inferred
|
||||
/// from the base URI. An absolute path replaces the base URI's path, while a
|
||||
/// relative path is appended lexically. Windows root-relative paths retain
|
||||
/// the base drive or UNC share, while drive-relative paths are rejected.
|
||||
/// Empty and `.` segments are ignored, while `..` removes one segment
|
||||
/// without escaping the URI root. Literal `%`, `?`, and `#` characters are
|
||||
/// percent-encoded as filename text. Paths containing a null character are
|
||||
/// rejected because they cannot be safely converted to native paths.
|
||||
/// without escaping the POSIX root, Windows drive, or UNC share. Literal
|
||||
/// `%`, `?`, and `#` characters are percent-encoded as filename text. Paths
|
||||
/// containing a null character are rejected because they cannot be safely
|
||||
/// converted to native paths.
|
||||
/// Opaque fallback URIs created by [`Self::from_abs_path`] reject non-empty
|
||||
/// joins.
|
||||
pub fn join(&self, path: &str) -> Result<Self, PathUriParseError> {
|
||||
if path.starts_with('/') {
|
||||
return Err(PathUriParseError::JoinPathMustBeRelative(path.to_string()));
|
||||
}
|
||||
if path.contains('\0') {
|
||||
return Err(PathUriParseError::InvalidFileUriPath {
|
||||
path: path.to_string(),
|
||||
@@ -231,6 +233,24 @@ impl PathUri {
|
||||
if path.is_empty() {
|
||||
return Ok(self.clone());
|
||||
}
|
||||
let convention =
|
||||
self.infer_path_convention()
|
||||
.ok_or_else(|| PathUriParseError::InvalidFileUriPath {
|
||||
path: self.to_string(),
|
||||
})?;
|
||||
// An absolute native path is already fully resolved, so replace the base URI's main path
|
||||
// instead of appending it.
|
||||
if let Ok(absolute) = LegacyAppPathString::from_str(path).to_path_uri(convention) {
|
||||
return Ok(absolute);
|
||||
}
|
||||
let path_bytes = path.as_bytes();
|
||||
if convention == PathConvention::Windows
|
||||
&& matches!(path_bytes, [drive, b':', ..] if drive.is_ascii_alphabetic())
|
||||
{
|
||||
return Err(PathUriParseError::InvalidFileUriPath {
|
||||
path: path.to_string(),
|
||||
});
|
||||
}
|
||||
if decode_bad_path_uri(&self.0).is_some() {
|
||||
return Err(PathUriParseError::InvalidFileUriPath {
|
||||
path: self.to_string(),
|
||||
@@ -238,19 +258,40 @@ impl PathUri {
|
||||
}
|
||||
|
||||
let mut url = self.0.clone();
|
||||
let anchor_depth = usize::from(convention == PathConvention::Windows);
|
||||
let mut depth = url
|
||||
.path_segments()
|
||||
.map(|segments| segments.filter(|segment| !segment.is_empty()).count())
|
||||
.unwrap_or_default();
|
||||
let windows_root_relative = convention == PathConvention::Windows
|
||||
&& matches!(path_bytes, [b'\\' | b'/', rest @ ..] if !matches!(rest, [b'\\' | b'/', ..]));
|
||||
{
|
||||
let Ok(mut segments) = url.path_segments_mut() else {
|
||||
unreachable!("validated file URLs support hierarchical path segments");
|
||||
};
|
||||
segments.pop_if_empty();
|
||||
if windows_root_relative {
|
||||
while depth > anchor_depth {
|
||||
segments.pop();
|
||||
depth -= 1;
|
||||
}
|
||||
}
|
||||
let path = match convention {
|
||||
PathConvention::Posix => path.to_string(),
|
||||
PathConvention::Windows => path.replace('\\', "/"),
|
||||
};
|
||||
for component in path.split('/') {
|
||||
match component {
|
||||
"" | "." => {}
|
||||
".." => {
|
||||
segments.pop();
|
||||
if depth > anchor_depth {
|
||||
segments.pop();
|
||||
depth -= 1;
|
||||
}
|
||||
}
|
||||
component => {
|
||||
segments.push(component);
|
||||
depth += 1;
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -350,6 +391,12 @@ impl TryFrom<String> for PathUri {
|
||||
}
|
||||
}
|
||||
|
||||
impl From<AbsolutePathBuf> for PathUri {
|
||||
fn from(p: AbsolutePathBuf) -> Self {
|
||||
Self::from_abs_path(&p)
|
||||
}
|
||||
}
|
||||
|
||||
impl<'de> Deserialize<'de> for PathUri {
|
||||
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
|
||||
where
|
||||
|
||||
Reference in new issue
Block a user