mirror of
https://github.com/openai/codex.git
synced 2026-11-13 22:47:20 +00:00
Resolve MCP file uploads with environment-native paths (#36002)
## Why File arguments for MCP app tools can use a path convention that differs from the host running Codex. Resolving those arguments with host-native path handling can therefore point uploads at the wrong file. ## What changed - Resolve file arguments against the selected turn environment's working directory with `PathUri::join`. - Use the resolved URI's inferred native spelling for errors and derive the uploaded filename without converting through a host-native path. ## Testing Run the existing MCP file upload integration test under Wine and reduce its streaming fixture to 2 MiB. GitOrigin-RevId: 0a659c19e73f5a26ddfc864e39dad6a4a404057b
This commit is contained in:
1 parent
a05bcda3db
commit
7579a2b413
2 files changed
+16
-19
No files matched your search
@@ -17,7 +17,6 @@ use codex_api::OPENAI_FILE_UPLOAD_LIMIT_BYTES;
|
|||||||
use codex_api::upload_openai_file;
|
use codex_api::upload_openai_file;
|
||||||
use codex_http_client::RouteAwareClientPool;
|
use codex_http_client::RouteAwareClientPool;
|
||||||
use codex_login::CodexAuth;
|
use codex_login::CodexAuth;
|
||||||
use codex_utils_path_uri::PathUri;
|
|
||||||
use serde_json::Value as JsonValue;
|
use serde_json::Value as JsonValue;
|
||||||
use std::collections::HashMap;
|
use std::collections::HashMap;
|
||||||
|
|
||||||
@@ -139,14 +138,10 @@ async fn build_uploaded_argument_value(
|
|||||||
"no primary turn environment is available".to_string(),
|
"no primary turn environment is available".to_string(),
|
||||||
));
|
));
|
||||||
};
|
};
|
||||||
// TODO(anp): Resolve app tool file arguments using the selected environment's native path
|
let path_uri = turn_environment
|
||||||
// convention so uploads can read relative paths from foreign environments.
|
|
||||||
let native_environment_cwd = turn_environment
|
|
||||||
.cwd()
|
.cwd()
|
||||||
.to_abs_path()
|
.join(file_path)
|
||||||
.map_err(|error| contextualize_error(error.to_string()))?;
|
.map_err(|error| contextualize_error(error.to_string()))?;
|
||||||
let resolved_path = native_environment_cwd.join(file_path);
|
|
||||||
let path_uri = PathUri::from_abs_path(&resolved_path);
|
|
||||||
let fs = turn_environment.environment.get_filesystem();
|
let fs = turn_environment.environment.get_filesystem();
|
||||||
let metadata = fs
|
let metadata = fs
|
||||||
.get_metadata(&path_uri, /*sandbox*/ None)
|
.get_metadata(&path_uri, /*sandbox*/ None)
|
||||||
@@ -155,13 +150,13 @@ async fn build_uploaded_argument_value(
|
|||||||
if !metadata.is_file {
|
if !metadata.is_file {
|
||||||
return Err(contextualize_error(format!(
|
return Err(contextualize_error(format!(
|
||||||
"path `{}` is not a file",
|
"path `{}` is not a file",
|
||||||
resolved_path.display()
|
path_uri.inferred_native_path_string()
|
||||||
)));
|
)));
|
||||||
}
|
}
|
||||||
if metadata.size > OPENAI_FILE_UPLOAD_LIMIT_BYTES {
|
if metadata.size > OPENAI_FILE_UPLOAD_LIMIT_BYTES {
|
||||||
return Err(contextualize_error(format!(
|
return Err(contextualize_error(format!(
|
||||||
"file `{}` is too large: {} bytes exceeds the limit of {} bytes",
|
"file `{}` is too large: {} bytes exceeds the limit of {} bytes",
|
||||||
resolved_path.display(),
|
path_uri.inferred_native_path_string(),
|
||||||
metadata.size,
|
metadata.size,
|
||||||
OPENAI_FILE_UPLOAD_LIMIT_BYTES,
|
OPENAI_FILE_UPLOAD_LIMIT_BYTES,
|
||||||
)));
|
)));
|
||||||
@@ -170,11 +165,17 @@ async fn build_uploaded_argument_value(
|
|||||||
.read_file_stream(&path_uri, /*sandbox*/ None)
|
.read_file_stream(&path_uri, /*sandbox*/ None)
|
||||||
.await
|
.await
|
||||||
.map_err(|error| contextualize_error(error.to_string()))?;
|
.map_err(|error| contextualize_error(error.to_string()))?;
|
||||||
let file_name = resolved_path
|
let file_name = path_uri
|
||||||
.file_name()
|
.basename()
|
||||||
.and_then(|value| value.to_str())
|
.or_else(|| {
|
||||||
.unwrap_or("file")
|
path_uri.infer_path_convention().and_then(|convention| {
|
||||||
.to_string();
|
convention
|
||||||
|
.path_segments(file_path)
|
||||||
|
.rfind(|segment| !segment.is_empty())
|
||||||
|
.map(str::to_string)
|
||||||
|
})
|
||||||
|
})
|
||||||
|
.unwrap_or_else(|| "file".to_string());
|
||||||
let upload_auth = codex_model_provider::auth_provider_from_auth(auth);
|
let upload_auth = codex_model_provider::auth_provider_from_auth(auth);
|
||||||
let uploaded = upload_openai_file(
|
let uploaded = upload_openai_file(
|
||||||
turn_context.config.chatgpt_base_url.trim_end_matches('/'),
|
turn_context.config.chatgpt_base_url.trim_end_matches('/'),
|
||||||
|
|||||||
@@ -27,7 +27,6 @@ use core_test_support::responses::mount_sse_sequence;
|
|||||||
use core_test_support::responses::namespace_child_tool;
|
use core_test_support::responses::namespace_child_tool;
|
||||||
use core_test_support::responses::sse;
|
use core_test_support::responses::sse;
|
||||||
use core_test_support::responses::start_mock_server;
|
use core_test_support::responses::start_mock_server;
|
||||||
use core_test_support::skip_if_wine_exec;
|
|
||||||
use core_test_support::test_codex::TestCodex;
|
use core_test_support::test_codex::TestCodex;
|
||||||
use pretty_assertions::assert_eq;
|
use pretty_assertions::assert_eq;
|
||||||
use serde_json::Value;
|
use serde_json::Value;
|
||||||
@@ -40,7 +39,7 @@ use wiremock::matchers::header;
|
|||||||
use wiremock::matchers::method;
|
use wiremock::matchers::method;
|
||||||
use wiremock::matchers::path;
|
use wiremock::matchers::path;
|
||||||
|
|
||||||
const STREAMED_FILE_SIZE: usize = 13 * 1024 * 1024;
|
const STREAMED_FILE_SIZE: usize = 2 * 1024 * 1024;
|
||||||
|
|
||||||
fn write_post_tool_use_hook(home: &Path) -> Result<()> {
|
fn write_post_tool_use_hook(home: &Path) -> Result<()> {
|
||||||
let script_path = home.join("post_tool_use_hook.py");
|
let script_path = home.join("post_tool_use_hook.py");
|
||||||
@@ -181,9 +180,6 @@ async fn run_extract_turn(test: &TestCodex, server: &MockServer) -> Result<Respo
|
|||||||
|
|
||||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
async fn codex_apps_file_params_omit_fields_absent_from_tool_schema() -> Result<()> {
|
async fn codex_apps_file_params_omit_fields_absent_from_tool_schema() -> Result<()> {
|
||||||
// TODO(anp): Remove after file-upload fixtures support target-native Windows paths.
|
|
||||||
skip_if_wine_exec!(Ok(()), "uses a host-native file-upload path");
|
|
||||||
|
|
||||||
let server = start_mock_server().await;
|
let server = start_mock_server().await;
|
||||||
let apps_server = AppsTestServer::mount(&server).await?;
|
let apps_server = AppsTestServer::mount(&server).await?;
|
||||||
mount_file_upload_mocks(&server, STREAMED_FILE_SIZE as u64).await;
|
mount_file_upload_mocks(&server, STREAMED_FILE_SIZE as u64).await;
|
||||||
|
|||||||
Reference in new issue
Block a user