From 3e791352a432fc51d7502c03f999e02b8528443b Mon Sep 17 00:00:00 2001 From: Liang-Ting Jiang Date: Thu, 28 May 2026 14:10:53 -0700 Subject: [PATCH] Surface Library uploads in computed MCP approval --- codex-rs/codex-api/src/files.rs | 92 ++++++++++++++++++-- codex-rs/codex-mcp/src/codex_apps.rs | 2 +- codex-rs/core/src/mcp_openai_file.rs | 65 ++++++++++++-- codex-rs/core/src/mcp_tool_call.rs | 79 +++++++++++++++-- codex-rs/core/src/mcp_tool_call_tests.rs | 103 ++++++++++++++++++++++- 5 files changed, 313 insertions(+), 28 deletions(-) diff --git a/codex-rs/codex-api/src/files.rs b/codex-rs/codex-api/src/files.rs index d1e2840066..438d76c49b 100644 --- a/codex-rs/codex-api/src/files.rs +++ b/codex-rs/codex-api/src/files.rs @@ -98,6 +98,7 @@ pub async fn upload_local_file( base_url: &str, auth: &dyn AuthProvider, path: &Path, + store_in_oai_library: bool, ) -> Result { let metadata = tokio::fs::metadata(path) .await @@ -129,12 +130,17 @@ pub async fn upload_local_file( .unwrap_or("file") .to_string(); let create_url = format!("{}/files", base_url.trim_end_matches('/')); + let mut create_body = serde_json::json!({ + "file_name": file_name, + "file_size": metadata.len(), + "use_case": OPENAI_FILE_USE_CASE, + }); + if store_in_oai_library { + create_body["store_in_library"] = serde_json::Value::Bool(true); + } + let create_response = authorized_request(auth, reqwest::Method::POST, &create_url) - .json(&serde_json::json!({ - "file_name": file_name, - "file_size": metadata.len(), - "use_case": OPENAI_FILE_USE_CASE, - })) + .json(&create_body) .send() .await .map_err(|source| OpenAiFileError::Request { @@ -363,9 +369,14 @@ mod tests { let path = dir.path().join("hello.txt"); tokio::fs::write(&path, b"hello").await.expect("write file"); - let uploaded = upload_local_file(&base_url, &chatgpt_auth(), &path) - .await - .expect("upload succeeds"); + let uploaded = upload_local_file( + &base_url, + &chatgpt_auth(), + &path, + /*store_in_oai_library*/ false, + ) + .await + .expect("upload succeeds"); assert_eq!(uploaded.file_id, "file_123"); assert_eq!(uploaded.uri, "sediment://file_123"); @@ -377,4 +388,69 @@ mod tests { assert_eq!(uploaded.mime_type, Some("text/plain".to_string())); assert_eq!(finalize_attempts.load(Ordering::SeqCst), 2); } + + #[tokio::test] + async fn upload_local_file_can_request_oai_library_storage() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/backend-api/files")) + .and(header("chatgpt-account-id", "account_id")) + .and(body_json(serde_json::json!({ + "file_name": "library-note.txt", + "file_size": 7, + "use_case": "codex", + "store_in_library": true, + }))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "file_id": "file_library", + "upload_url": format!("{}/upload/file_library", server.uri()), + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("PUT")) + .and(path("/upload/file_library")) + .and(header("content-length", "7")) + .respond_with(ResponseTemplate::new(200)) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/backend-api/files/file_library/uploaded")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "status": "success", + "download_url": format!("{}/download/file_library", server.uri()), + "file_name": "library-note.txt", + "mime_type": "text/plain", + "file_size_bytes": 7, + }))) + .expect(1) + .mount(&server) + .await; + + let base_url = base_url_for(&server); + let dir = TempDir::new().expect("temp dir"); + let path = dir.path().join("library-note.txt"); + tokio::fs::write(&path, b"library") + .await + .expect("write file"); + + let uploaded = upload_local_file( + &base_url, + &chatgpt_auth(), + &path, + /*store_in_oai_library*/ true, + ) + .await + .expect("upload succeeds"); + + assert_eq!(uploaded.file_id, "file_library"); + assert_eq!(uploaded.uri, "sediment://file_library"); + assert_eq!( + uploaded.download_url, + format!("{}/download/file_library", server.uri()) + ); + + server.verify().await; + } } diff --git a/codex-rs/codex-mcp/src/codex_apps.rs b/codex-rs/codex-mcp/src/codex_apps.rs index 3196a143e8..26de982fcb 100644 --- a/codex-rs/codex-mcp/src/codex_apps.rs +++ b/codex-rs/codex-mcp/src/codex_apps.rs @@ -303,7 +303,7 @@ struct CodexAppsServerInfoDiskCache { } const CODEX_APPS_TOOLS_CACHE_DIR: &str = "cache/codex_apps_tools"; -pub(crate) const CODEX_APPS_TOOLS_CACHE_SCHEMA_VERSION: u8 = 3; +pub(crate) const CODEX_APPS_TOOLS_CACHE_SCHEMA_VERSION: u8 = 4; const CODEX_APPS_SERVER_INFO_CACHE_DIR: &str = "cache/codex_apps_server_info"; const CODEX_APPS_SERVER_INFO_CACHE_SCHEMA_VERSION: u8 = 1; diff --git a/codex-rs/core/src/mcp_openai_file.rs b/codex-rs/core/src/mcp_openai_file.rs index ae44515c62..7720f5a034 100644 --- a/codex-rs/core/src/mcp_openai_file.rs +++ b/codex-rs/core/src/mcp_openai_file.rs @@ -3,6 +3,9 @@ //! Strategy: //! - Inspect `_meta["openai/fileParams"]` to discover which tool arguments are //! file inputs. +//! - Inspect validated `_meta["openai/fileUploadConfig"]` metadata to discover +//! host-computed Library storage for the approval action before local files +//! are uploaded. //! - At tool execution time, upload those local files to OpenAI file storage //! and rewrite only the declared arguments into the provided-file payload //! shape expected by the downstream Apps tool. @@ -16,11 +19,17 @@ use codex_api::upload_local_file; use codex_login::CodexAuth; use serde_json::Value as JsonValue; +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct OpenAIFileInputParams { + pub(crate) names: Vec, + pub(crate) store_in_oai_library: bool, +} + pub(crate) async fn rewrite_mcp_tool_arguments_for_openai_files( sess: &Session, turn_context: &TurnContext, arguments_value: Option, - openai_file_input_params: Option<&[String]>, + openai_file_input_params: Option<&OpenAIFileInputParams>, ) -> Result, String> { let Some(openai_file_input_params) = openai_file_input_params else { return Ok(arguments_value); @@ -34,14 +43,21 @@ pub(crate) async fn rewrite_mcp_tool_arguments_for_openai_files( }; let auth = sess.services.auth_manager.auth().await; let mut rewritten_arguments = arguments.clone(); + let store_in_oai_library = + store_in_oai_library_for_openai_file_upload(openai_file_input_params); - for field_name in openai_file_input_params { + for field_name in &openai_file_input_params.names { let Some(value) = arguments.get(field_name) else { continue; }; - let Some(uploaded_value) = - rewrite_argument_value_for_openai_files(turn_context, auth.as_ref(), field_name, value) - .await? + let Some(uploaded_value) = rewrite_argument_value_for_openai_files( + turn_context, + auth.as_ref(), + field_name, + value, + store_in_oai_library, + ) + .await? else { continue; }; @@ -55,11 +71,18 @@ pub(crate) async fn rewrite_mcp_tool_arguments_for_openai_files( Ok(Some(JsonValue::Object(rewritten_arguments))) } +fn store_in_oai_library_for_openai_file_upload( + openai_file_input_params: &OpenAIFileInputParams, +) -> bool { + openai_file_input_params.store_in_oai_library +} + async fn rewrite_argument_value_for_openai_files( turn_context: &TurnContext, auth: Option<&CodexAuth>, field_name: &str, value: &JsonValue, + store_in_oai_library: bool, ) -> Result, String> { match value { JsonValue::String(path_or_file_ref) => { @@ -69,6 +92,7 @@ async fn rewrite_argument_value_for_openai_files( field_name, /*index*/ None, path_or_file_ref, + store_in_oai_library, ) .await?; Ok(Some(rewritten)) @@ -85,6 +109,7 @@ async fn rewrite_argument_value_for_openai_files( field_name, Some(index), path_or_file_ref, + store_in_oai_library, ) .await?; rewritten_values.push(rewritten); @@ -101,6 +126,7 @@ async fn build_uploaded_local_argument_value( field_name: &str, index: Option, file_path: &str, + store_in_oai_library: bool, ) -> Result { #[allow(deprecated)] let resolved_path = turn_context.resolve_path(Some(file_path.to_string())); @@ -119,6 +145,7 @@ async fn build_uploaded_local_argument_value( turn_context.config.chatgpt_base_url.trim_end_matches('/'), upload_auth.as_ref(), &resolved_path, + store_in_oai_library, ) .await .map_err(|error| match index { @@ -165,8 +192,25 @@ mod tests { assert_eq!(rewritten, arguments); } + #[test] + fn store_in_oai_library_comes_from_computed_approval_action() { + let openai_file_input_params = OpenAIFileInputParams { + names: Vec::new(), + store_in_oai_library: true, + }; + assert!(store_in_oai_library_for_openai_file_upload( + &openai_file_input_params + )); + assert!(!store_in_oai_library_for_openai_file_upload( + &OpenAIFileInputParams { + names: Vec::new(), + store_in_oai_library: false, + }, + )); + } + #[tokio::test] - async fn build_uploaded_local_argument_value_uploads_local_file_path() { + async fn build_uploaded_local_argument_value_honors_upload_options() { use wiremock::Mock; use wiremock::MockServer; use wiremock::ResponseTemplate; @@ -183,6 +227,7 @@ mod tests { "file_name": "file_report.csv", "file_size": 5, "use_case": "codex", + "store_in_library": true, }))) .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ "file_id": "file_123", @@ -232,6 +277,7 @@ mod tests { "file", /*index*/ None, "file_report.csv", + /*store_in_oai_library*/ true, ) .await .expect("rewrite should upload the local file"); @@ -314,6 +360,7 @@ mod tests { Some(&auth), "file", &serde_json::json!("file_report.csv"), + /*store_in_oai_library*/ false, ) .await .expect("rewrite should succeed"); @@ -431,6 +478,7 @@ mod tests { Some(&auth), "files", &serde_json::json!(["one.csv", "two.csv"]), + /*store_in_oai_library*/ false, ) .await .expect("rewrite should succeed"); @@ -470,7 +518,10 @@ mod tests { Some(serde_json::json!({ "file": "/definitely/missing/file.csv", })), - Some(&["file".to_string()]), + Some(&OpenAIFileInputParams { + names: vec!["file".to_string()], + store_in_oai_library: false, + }), ) .await .expect_err("missing file should fail"); diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index e514fa0d03..586414187b 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -15,6 +15,7 @@ use crate::guardian::new_guardian_review_id; use crate::guardian::review_approval_request; use crate::guardian::routes_approval_to_guardian; use crate::hook_runtime::run_permission_request_hooks; +use crate::mcp_openai_file::OpenAIFileInputParams; use crate::mcp_openai_file::rewrite_mcp_tool_arguments_for_openai_files; use crate::mcp_tool_approval_templates::RenderedMcpToolApprovalParam; use crate::mcp_tool_approval_templates::render_mcp_tool_approval_template; @@ -163,12 +164,19 @@ pub(crate) async fn handle_mcp_tool_call( } else { connectors::AppToolPolicy::default() }; - let approval_mode = if server == CODEX_APPS_MCP_SERVER_NAME { + let approval_mode = if metadata + .as_ref() + .and_then(|metadata| metadata.openai_file_input_params.as_ref()) + .is_some_and(|params| params.store_in_oai_library) + { + AppToolApproval::Prompt + } else if server == CODEX_APPS_MCP_SERVER_NAME { app_tool_policy.approval } else { custom_mcp_tool_approval_mode(sess.as_ref(), turn_context.as_ref(), &server, &tool_name) .await }; + let approval_invocation = approval_visible_mcp_invocation(&invocation, metadata.as_ref()); if server == CODEX_APPS_MCP_SERVER_NAME && !app_tool_policy.enabled { let result = notify_mcp_tool_call_skip( @@ -204,7 +212,7 @@ pub(crate) async fn handle_mcp_tool_call( sess.as_ref(), turn_context.as_ref(), &call_id, - invocation.clone(), + approval_invocation.clone(), item_metadata.clone(), ) .await; @@ -213,7 +221,7 @@ pub(crate) async fn handle_mcp_tool_call( &sess, turn_context, &call_id, - &invocation, + &approval_invocation, &hook_tool_name, metadata.as_ref(), approval_mode, @@ -328,7 +336,7 @@ async fn handle_approved_mcp_tool_call( sess, turn_context, arguments_value.clone(), - metadata.and_then(|metadata| metadata.openai_file_input_params.as_deref()), + metadata.and_then(|metadata| metadata.openai_file_input_params.as_ref()), ) .await; let tool_input = match &rewrite { @@ -976,13 +984,15 @@ pub(crate) struct McpToolApprovalMetadata { tool_description: Option, mcp_app_resource_uri: Option, codex_apps_meta: Option>, - openai_file_input_params: Option>, + openai_file_input_params: Option, } const MCP_TOOL_OPENAI_OUTPUT_TEMPLATE_META_KEY: &str = "openai/outputTemplate"; const MCP_TOOL_UI_RESOURCE_URI_META_KEY: &str = "ui/resourceUri"; const MCP_TOOL_PLUGIN_ID_META_KEY: &str = "plugin_id"; const MCP_TOOL_THREAD_ID_META_KEY: &str = "threadId"; +const MCP_TOOL_OPENAI_FILE_UPLOAD_CONFIG_META_KEY: &str = "openai/fileUploadConfig"; +const MCP_TOOL_OPENAI_FILE_UPLOAD_STORE_IN_OAI_LIBRARY_KEY: &str = "store_in_oai_library"; async fn custom_mcp_tool_approval_mode( sess: &Session, @@ -1464,6 +1474,7 @@ pub(crate) async fn lookup_mcp_tool_metadata( openai_file_input_params: openai_file_input_params_for_server( server, tool_info.tool.meta.as_deref(), + tool_info.tool.input_schema.as_ref(), ), }) } @@ -1471,10 +1482,60 @@ pub(crate) async fn lookup_mcp_tool_metadata( fn openai_file_input_params_for_server( server: &str, meta: Option<&serde_json::Map>, -) -> Option> { - (server == CODEX_APPS_MCP_SERVER_NAME) - .then_some(declared_openai_file_input_param_names(meta)) - .filter(|params| !params.is_empty()) + _input_schema: &serde_json::Map, +) -> Option { + if server != CODEX_APPS_MCP_SERVER_NAME { + return None; + } + + let names = declared_openai_file_input_param_names(meta); + if names.is_empty() { + return None; + } + + Some(OpenAIFileInputParams { + names, + store_in_oai_library: declared_openai_file_upload_store_in_oai_library(meta), + }) +} + +fn declared_openai_file_upload_store_in_oai_library( + meta: Option<&serde_json::Map>, +) -> bool { + meta.and_then(|meta| meta.get(MCP_TOOL_OPENAI_FILE_UPLOAD_CONFIG_META_KEY)) + .and_then(serde_json::Value::as_object) + .and_then(|config| config.get(MCP_TOOL_OPENAI_FILE_UPLOAD_STORE_IN_OAI_LIBRARY_KEY)) + .and_then(serde_json::Value::as_bool) + .unwrap_or(false) +} + +fn approval_visible_mcp_invocation( + invocation: &McpInvocation, + metadata: Option<&McpToolApprovalMetadata>, +) -> McpInvocation { + let Some(openai_file_input_params) = + metadata.and_then(|metadata| metadata.openai_file_input_params.as_ref()) + else { + return invocation.clone(); + }; + if !openai_file_input_params.store_in_oai_library { + return invocation.clone(); + } + + let mut approval_arguments = invocation + .arguments + .as_ref() + .and_then(serde_json::Value::as_object) + .cloned() + .unwrap_or_default(); + approval_arguments.insert( + MCP_TOOL_OPENAI_FILE_UPLOAD_STORE_IN_OAI_LIBRARY_KEY.to_string(), + serde_json::Value::Bool(true), + ); + McpInvocation { + arguments: Some(serde_json::Value::Object(approval_arguments)), + ..invocation.clone() + } } fn get_mcp_app_resource_uri( diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 534c2f0118..56b12a7d3a 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -344,17 +344,114 @@ fn openai_file_params_are_only_honored_for_codex_apps() { "openai/fileParams": ["file"], }); let meta = meta.as_object(); + let input_schema = serde_json::json!({ + "type": "object", + "properties": {}, + }); + let input_schema = input_schema.as_object().expect("input schema object"); assert_eq!( - openai_file_input_params_for_server(CODEX_APPS_MCP_SERVER_NAME, meta), - Some(vec!["file".to_string()]) + openai_file_input_params_for_server(CODEX_APPS_MCP_SERVER_NAME, meta, input_schema), + Some(OpenAIFileInputParams { + names: vec!["file".to_string()], + store_in_oai_library: false, + }) ); assert_eq!( - openai_file_input_params_for_server("minimaltest", meta), + openai_file_input_params_for_server("minimaltest", meta, input_schema), None ); } +#[test] +fn openai_file_upload_config_marks_host_computed_library_upload() { + let valid_meta_value = serde_json::json!({ + "openai/fileParams": ["file"], + "openai/fileUploadConfig": { + "store_in_oai_library": true, + }, + }); + let valid_meta = valid_meta_value.as_object(); + let valid_input_schema_value = serde_json::json!({ + "type": "object", + "properties": {}, + }); + let valid_input_schema = valid_input_schema_value + .as_object() + .expect("input schema object"); + + assert_eq!( + openai_file_input_params_for_server( + CODEX_APPS_MCP_SERVER_NAME, + valid_meta, + valid_input_schema, + ), + Some(OpenAIFileInputParams { + names: vec!["file".to_string()], + store_in_oai_library: true, + }) + ); + + for (meta, input_schema) in [( + serde_json::json!({ + "openai/fileParams": ["file"], + "openai/fileUploadConfig": { + "store_in_oai_library_arg": "store_in_oai_library", + }, + }), + valid_input_schema_value.clone(), + )] { + assert_eq!( + openai_file_input_params_for_server( + CODEX_APPS_MCP_SERVER_NAME, + meta.as_object(), + input_schema.as_object().expect("input schema object"), + ), + Some(OpenAIFileInputParams { + names: vec!["file".to_string()], + store_in_oai_library: false, + }) + ); + } +} + +#[test] +fn approval_visible_mcp_invocation_surfaces_host_computed_library_upload() { + let invocation = McpInvocation { + server: CODEX_APPS_MCP_SERVER_NAME.to_string(), + tool: "library_create_library_file".to_string(), + arguments: Some(serde_json::json!({ + "file": "/tmp/library.txt", + })), + }; + let metadata = McpToolApprovalMetadata { + annotations: None, + connector_id: None, + connector_name: None, + connector_description: None, + plugin_id: None, + tool_title: None, + tool_description: None, + mcp_app_resource_uri: None, + codex_apps_meta: None, + openai_file_input_params: Some(OpenAIFileInputParams { + names: vec!["file".to_string()], + store_in_oai_library: true, + }), + }; + + assert_eq!( + approval_visible_mcp_invocation(&invocation, Some(&metadata)), + McpInvocation { + arguments: Some(serde_json::json!({ + "file": "/tmp/library.txt", + "store_in_oai_library": true, + })), + ..invocation + } + ); +} + #[test] fn approval_required_when_read_only_false_and_destructive() { let annotations = annotations(Some(false), Some(true), /*open_world*/ None);