mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] Route MCP file uploads through environment filesystem (#27923)
## Why Codex Apps tools can mark arguments with `openai/fileParams`, but the execution path resolved and opened those files directly on the host. That bypassed the selected turn environment and prevented annotated file arguments from working with remote environments. ## What changed - resolve annotated file arguments against the primary turn environment - read file metadata and contents through that environment's sandboxed `ExecutorFileSystem` - reject files over the 512 MiB limit from metadata before reading or transferring them - retain the buffered upload-size check as defense in depth - make the OpenAI upload API accept a filename and buffered contents instead of owning local filesystem access - describe the model-visible argument as a path in the primary environment This builds on #27927, which added `size` to internal filesystem metadata. ## Testing - `just test -p codex-api upload_openai_file_returns_canonical_uri` - `just test -p codex-mcp tool_with_model_visible_input_schema_masks_file_params` - `just test -p codex-core mcp_openai_file` - `just test -p codex-core codex_apps_file_params_upload_environment_files_before_mcp_tool_call`
This commit is contained in:
@@ -3,7 +3,8 @@
|
||||
//! Strategy:
|
||||
//! - Inspect `_meta["openai/fileParams"]` to discover which tool arguments are
|
||||
//! file inputs.
|
||||
//! - At tool execution time, upload those local files to OpenAI file storage
|
||||
//! - At tool execution time, read those files from the primary environment,
|
||||
//! upload them to OpenAI file storage,
|
||||
//! and rewrite only the declared arguments into the provided-file payload
|
||||
//! shape expected by the downstream Apps tool.
|
||||
//!
|
||||
@@ -12,8 +13,10 @@
|
||||
|
||||
use crate::session::session::Session;
|
||||
use crate::session::turn_context::TurnContext;
|
||||
use codex_api::upload_local_file;
|
||||
use codex_api::OPENAI_FILE_UPLOAD_LIMIT_BYTES;
|
||||
use codex_api::upload_openai_file;
|
||||
use codex_login::CodexAuth;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use serde_json::Value as JsonValue;
|
||||
|
||||
pub(crate) async fn rewrite_mcp_tool_arguments_for_openai_files(
|
||||
@@ -62,13 +65,13 @@ async fn rewrite_argument_value_for_openai_files(
|
||||
value: &JsonValue,
|
||||
) -> Result<Option<JsonValue>, String> {
|
||||
match value {
|
||||
JsonValue::String(path_or_file_ref) => {
|
||||
let rewritten = build_uploaded_local_argument_value(
|
||||
JsonValue::String(file_path) => {
|
||||
let rewritten = build_uploaded_argument_value(
|
||||
turn_context,
|
||||
auth,
|
||||
field_name,
|
||||
/*index*/ None,
|
||||
path_or_file_ref,
|
||||
file_path,
|
||||
)
|
||||
.await?;
|
||||
Ok(Some(rewritten))
|
||||
@@ -76,15 +79,15 @@ async fn rewrite_argument_value_for_openai_files(
|
||||
JsonValue::Array(values) => {
|
||||
let mut rewritten_values = Vec::with_capacity(values.len());
|
||||
for (index, item) in values.iter().enumerate() {
|
||||
let Some(path_or_file_ref) = item.as_str() else {
|
||||
let Some(file_path) = item.as_str() else {
|
||||
return Ok(None);
|
||||
};
|
||||
let rewritten = build_uploaded_local_argument_value(
|
||||
let rewritten = build_uploaded_argument_value(
|
||||
turn_context,
|
||||
auth,
|
||||
field_name,
|
||||
Some(index),
|
||||
path_or_file_ref,
|
||||
file_path,
|
||||
)
|
||||
.await?;
|
||||
rewritten_values.push(rewritten);
|
||||
@@ -95,38 +98,70 @@ async fn rewrite_argument_value_for_openai_files(
|
||||
}
|
||||
}
|
||||
|
||||
async fn build_uploaded_local_argument_value(
|
||||
async fn build_uploaded_argument_value(
|
||||
turn_context: &TurnContext,
|
||||
auth: Option<&CodexAuth>,
|
||||
field_name: &str,
|
||||
index: Option<usize>,
|
||||
file_path: &str,
|
||||
) -> Result<JsonValue, String> {
|
||||
#[allow(deprecated)]
|
||||
let resolved_path = turn_context.resolve_path(Some(file_path.to_string()));
|
||||
let Some(auth) = auth else {
|
||||
return Err(
|
||||
"ChatGPT auth is required to upload local files for Codex Apps tools".to_string(),
|
||||
);
|
||||
};
|
||||
if !auth.uses_codex_backend() {
|
||||
return Err(
|
||||
"ChatGPT auth is required to upload local files for Codex Apps tools".to_string(),
|
||||
);
|
||||
}
|
||||
let upload_auth = codex_model_provider::auth_provider_from_auth(auth);
|
||||
let uploaded = upload_local_file(
|
||||
turn_context.config.chatgpt_base_url.trim_end_matches('/'),
|
||||
upload_auth.as_ref(),
|
||||
&resolved_path,
|
||||
)
|
||||
.await
|
||||
.map_err(|error| match index {
|
||||
let contextualize_error = |error: String| match index {
|
||||
Some(index) => {
|
||||
format!("failed to upload `{file_path}` for `{field_name}[{index}]`: {error}")
|
||||
}
|
||||
None => format!("failed to upload `{file_path}` for `{field_name}`: {error}"),
|
||||
})?;
|
||||
};
|
||||
let Some(auth) = auth else {
|
||||
return Err("ChatGPT auth is required to upload files for Codex Apps tools".to_string());
|
||||
};
|
||||
if !auth.uses_codex_backend() {
|
||||
return Err("ChatGPT auth is required to upload files for Codex Apps tools".to_string());
|
||||
}
|
||||
let Some(turn_environment) = turn_context.environments.primary() else {
|
||||
return Err(contextualize_error(
|
||||
"no primary turn environment is available".to_string(),
|
||||
));
|
||||
};
|
||||
let resolved_path = turn_environment.cwd().join(file_path);
|
||||
let path_uri = PathUri::from_abs_path(&resolved_path);
|
||||
let fs = turn_environment.environment.get_filesystem();
|
||||
let metadata = fs
|
||||
.get_metadata(&path_uri, /*sandbox*/ None)
|
||||
.await
|
||||
.map_err(|error| contextualize_error(error.to_string()))?;
|
||||
if !metadata.is_file {
|
||||
return Err(contextualize_error(format!(
|
||||
"path `{}` is not a file",
|
||||
resolved_path.display()
|
||||
)));
|
||||
}
|
||||
if metadata.size > OPENAI_FILE_UPLOAD_LIMIT_BYTES {
|
||||
return Err(contextualize_error(format!(
|
||||
"file `{}` is too large: {} bytes exceeds the limit of {} bytes",
|
||||
resolved_path.display(),
|
||||
metadata.size,
|
||||
OPENAI_FILE_UPLOAD_LIMIT_BYTES,
|
||||
)));
|
||||
}
|
||||
let contents = fs
|
||||
.read_file_stream(&path_uri, /*sandbox*/ None)
|
||||
.await
|
||||
.map_err(|error| contextualize_error(error.to_string()))?;
|
||||
let file_name = resolved_path
|
||||
.file_name()
|
||||
.and_then(|value| value.to_str())
|
||||
.unwrap_or("file")
|
||||
.to_string();
|
||||
let upload_auth = codex_model_provider::auth_provider_from_auth(auth);
|
||||
let uploaded = upload_openai_file(
|
||||
turn_context.config.chatgpt_base_url.trim_end_matches('/'),
|
||||
upload_auth.as_ref(),
|
||||
file_name,
|
||||
metadata.size,
|
||||
contents,
|
||||
)
|
||||
.await
|
||||
.map_err(|error| contextualize_error(error.to_string()))?;
|
||||
Ok(serde_json::json!({
|
||||
"download_url": uploaded.download_url,
|
||||
"file_id": uploaded.file_id,
|
||||
@@ -141,11 +176,29 @@ async fn build_uploaded_local_argument_value(
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::session::tests::make_session_and_context;
|
||||
use crate::session::turn_context::TurnEnvironment;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::path::Path;
|
||||
use std::sync::Arc;
|
||||
use tempfile::tempdir;
|
||||
|
||||
fn set_primary_environment_cwd(turn_context: &mut TurnContext, cwd: &Path) {
|
||||
let cwd = AbsolutePathBuf::try_from(cwd).expect("absolute path");
|
||||
turn_context.permission_profile = codex_protocol::models::PermissionProfile::Disabled;
|
||||
let primary = turn_context
|
||||
.environments
|
||||
.turn_environments
|
||||
.first_mut()
|
||||
.expect("primary environment");
|
||||
*primary = TurnEnvironment::new(
|
||||
primary.environment_id.clone(),
|
||||
Arc::clone(&primary.environment),
|
||||
cwd,
|
||||
primary.shell.clone(),
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn openai_file_argument_rewrite_requires_declared_file_params() {
|
||||
let (session, turn_context) = make_session_and_context().await;
|
||||
@@ -166,7 +219,7 @@ mod tests {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn build_uploaded_local_argument_value_uploads_local_file_path() {
|
||||
async fn build_uploaded_argument_value_uploads_environment_file() {
|
||||
use wiremock::Mock;
|
||||
use wiremock::MockServer;
|
||||
use wiremock::ResponseTemplate;
|
||||
@@ -217,16 +270,13 @@ mod tests {
|
||||
tokio::fs::write(&local_path, b"hello")
|
||||
.await
|
||||
.expect("write local file");
|
||||
#[allow(deprecated)]
|
||||
{
|
||||
turn_context.cwd = AbsolutePathBuf::try_from(dir.path()).expect("absolute path");
|
||||
}
|
||||
set_primary_environment_cwd(&mut turn_context, dir.path());
|
||||
|
||||
let mut config = (*turn_context.config).clone();
|
||||
config.chatgpt_base_url = format!("{}/backend-api", server.uri());
|
||||
turn_context.config = Arc::new(config);
|
||||
|
||||
let rewritten = build_uploaded_local_argument_value(
|
||||
let rewritten = build_uploaded_argument_value(
|
||||
&turn_context,
|
||||
Some(&auth),
|
||||
"file",
|
||||
@@ -249,6 +299,31 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn build_uploaded_argument_value_rejects_oversized_file_before_reading() {
|
||||
let (_, mut turn_context) = make_session_and_context().await;
|
||||
let auth = CodexAuth::create_dummy_chatgpt_auth_for_testing();
|
||||
let dir = tempdir().expect("temp dir");
|
||||
let file_path = dir.path().join("oversized.bin");
|
||||
let file = std::fs::File::create(&file_path).expect("create sparse file");
|
||||
file.set_len(OPENAI_FILE_UPLOAD_LIMIT_BYTES + 1)
|
||||
.expect("size sparse file");
|
||||
set_primary_environment_cwd(&mut turn_context, dir.path());
|
||||
|
||||
let error = build_uploaded_argument_value(
|
||||
&turn_context,
|
||||
Some(&auth),
|
||||
"file",
|
||||
/*index*/ None,
|
||||
"oversized.bin",
|
||||
)
|
||||
.await
|
||||
.expect_err("oversized file should be rejected");
|
||||
|
||||
assert!(error.contains("is too large"));
|
||||
assert!(error.contains(&(OPENAI_FILE_UPLOAD_LIMIT_BYTES + 1).to_string()));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn rewrite_argument_value_for_openai_files_rewrites_scalar_path() {
|
||||
use wiremock::Mock;
|
||||
@@ -301,10 +376,7 @@ mod tests {
|
||||
tokio::fs::write(&local_path, b"hello")
|
||||
.await
|
||||
.expect("write local file");
|
||||
#[allow(deprecated)]
|
||||
{
|
||||
turn_context.cwd = AbsolutePathBuf::try_from(dir.path()).expect("absolute path");
|
||||
}
|
||||
set_primary_environment_cwd(&mut turn_context, dir.path());
|
||||
|
||||
let mut config = (*turn_context.config).clone();
|
||||
config.chatgpt_base_url = format!("{}/backend-api", server.uri());
|
||||
@@ -418,10 +490,7 @@ mod tests {
|
||||
tokio::fs::write(dir.path().join("two.csv"), b"two")
|
||||
.await
|
||||
.expect("write second local file");
|
||||
#[allow(deprecated)]
|
||||
{
|
||||
turn_context.cwd = AbsolutePathBuf::try_from(dir.path()).expect("absolute path");
|
||||
}
|
||||
set_primary_environment_cwd(&mut turn_context, dir.path());
|
||||
|
||||
let mut config = (*turn_context.config).clone();
|
||||
config.chatgpt_base_url = format!("{}/backend-api", server.uri());
|
||||
|
||||
Reference in New Issue
Block a user