mirror of
https://github.com/openai/codex.git
synced 2026-08-23 13:09:46 +00:00
codex-api: route file uploads through HTTP client factory (#31363)
## Why Codex Apps file parameters use a three-step upload flow: create a file record, PUT bytes to a returned signed URL, and finalize the upload. Each step still constructed a default `reqwest` client, so the flow could bypass `features.respect_system_proxy` even after model API requests honored it. This stack entry makes the resolved client policy a required input to the upload API and resolves each concrete destination independently. ## What changed - Require `HttpClientFactory` in `upload_openai_file`. - Build clients for the create, signed upload, and finalize URLs through the shared API route policy. - Pass the factory derived from the turn configuration at the Apps/MCP call site. - Return a destination-aware `ClientBuild` error when enabled route selection cannot construct a client. - Preserve the legacy logged fallback for the feature-off `ReqwestDefault` policy. ## Review guide 1. `codex-api/src/files.rs` changes the upload API and centralizes route-aware client construction. 2. The three request stages each supply their actual URL, including the separately hosted signed blob URL. 3. `core/src/mcp_openai_file.rs` is the only production caller and supplies the turn configuration factory. ## Validation - `cargo check --tests -p codex-api -p codex-core` - `just test -p codex-api files` (1 matching upload test passed; 135 tests skipped by filter) - `just fix -p codex-api -p codex-core` ## Follow-up Other direct HTTP clients remain separate migration slices. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/31363). * #31637 * #31431 * __->__ #31363
This commit is contained in:
@@ -2,7 +2,10 @@ use std::time::Duration;
|
||||
|
||||
use crate::AuthProvider;
|
||||
use bytes::Bytes;
|
||||
use codex_http_client::build_reqwest_client_with_custom_ca;
|
||||
use codex_http_client::BuildRouteAwareHttpClientError;
|
||||
use codex_http_client::ClientRouteClass;
|
||||
use codex_http_client::HttpClientFactory;
|
||||
use codex_http_client::OutboundProxyPolicy;
|
||||
use futures::Stream;
|
||||
use reqwest::StatusCode;
|
||||
use reqwest::header::CONTENT_LENGTH;
|
||||
@@ -55,6 +58,12 @@ pub enum OpenAiFileError {
|
||||
#[source]
|
||||
source: serde_json::Error,
|
||||
},
|
||||
#[error("failed to build OpenAI file client for {url}: {source}")]
|
||||
ClientBuild {
|
||||
url: String,
|
||||
#[source]
|
||||
source: BuildRouteAwareHttpClientError,
|
||||
},
|
||||
#[error("OpenAI file upload for `{file_id}` is not ready yet")]
|
||||
UploadNotReady { file_id: String },
|
||||
#[error("OpenAI file upload for `{file_id}` failed: {message}")]
|
||||
@@ -84,6 +93,7 @@ pub fn openai_file_uri(file_id: &str) -> String {
|
||||
pub async fn upload_openai_file(
|
||||
base_url: &str,
|
||||
auth: &dyn AuthProvider,
|
||||
http_client_factory: &HttpClientFactory,
|
||||
file_name: String,
|
||||
file_size_bytes: u64,
|
||||
contents: impl Stream<Item = std::io::Result<Bytes>> + Send + 'static,
|
||||
@@ -97,18 +107,23 @@ pub async fn upload_openai_file(
|
||||
}
|
||||
|
||||
let create_url = format!("{}/files", base_url.trim_end_matches('/'));
|
||||
let create_response = authorized_request(auth, reqwest::Method::POST, &create_url)
|
||||
.json(&serde_json::json!({
|
||||
"file_name": file_name.as_str(),
|
||||
"file_size": file_size_bytes,
|
||||
"use_case": OPENAI_FILE_USE_CASE,
|
||||
}))
|
||||
.send()
|
||||
.await
|
||||
.map_err(|source| OpenAiFileError::Request {
|
||||
url: create_url.clone(),
|
||||
source,
|
||||
})?;
|
||||
let create_response = authorized_request(
|
||||
http_client_factory,
|
||||
auth,
|
||||
reqwest::Method::POST,
|
||||
&create_url,
|
||||
)?
|
||||
.json(&serde_json::json!({
|
||||
"file_name": file_name.as_str(),
|
||||
"file_size": file_size_bytes,
|
||||
"use_case": OPENAI_FILE_USE_CASE,
|
||||
}))
|
||||
.send()
|
||||
.await
|
||||
.map_err(|source| OpenAiFileError::Request {
|
||||
url: create_url.clone(),
|
||||
source,
|
||||
})?;
|
||||
let create_status = create_response.status();
|
||||
let create_body = create_response.text().await.unwrap_or_default();
|
||||
if !create_status.is_success() {
|
||||
@@ -124,7 +139,7 @@ pub async fn upload_openai_file(
|
||||
source,
|
||||
})?;
|
||||
|
||||
let upload_response = build_reqwest_client()
|
||||
let upload_response = build_reqwest_client(http_client_factory, &create_payload.upload_url)?
|
||||
.put(&create_payload.upload_url)
|
||||
.timeout(OPENAI_FILE_REQUEST_TIMEOUT)
|
||||
.header("x-ms-blob-type", "BlockBlob")
|
||||
@@ -153,14 +168,19 @@ pub async fn upload_openai_file(
|
||||
);
|
||||
let finalize_started_at = Instant::now();
|
||||
loop {
|
||||
let finalize_response = authorized_request(auth, reqwest::Method::POST, &finalize_url)
|
||||
.json(&serde_json::json!({}))
|
||||
.send()
|
||||
.await
|
||||
.map_err(|source| OpenAiFileError::Request {
|
||||
url: finalize_url.clone(),
|
||||
source,
|
||||
})?;
|
||||
let finalize_response = authorized_request(
|
||||
http_client_factory,
|
||||
auth,
|
||||
reqwest::Method::POST,
|
||||
&finalize_url,
|
||||
)?
|
||||
.json(&serde_json::json!({}))
|
||||
.send()
|
||||
.await
|
||||
.map_err(|source| OpenAiFileError::Request {
|
||||
url: finalize_url.clone(),
|
||||
source,
|
||||
})?;
|
||||
let finalize_status = finalize_response.status();
|
||||
let finalize_body = finalize_response.text().await.unwrap_or_default();
|
||||
if !finalize_status.is_success() {
|
||||
@@ -213,25 +233,45 @@ pub async fn upload_openai_file(
|
||||
}
|
||||
|
||||
fn authorized_request(
|
||||
http_client_factory: &HttpClientFactory,
|
||||
auth: &dyn AuthProvider,
|
||||
method: reqwest::Method,
|
||||
url: &str,
|
||||
) -> reqwest::RequestBuilder {
|
||||
) -> Result<reqwest::RequestBuilder, OpenAiFileError> {
|
||||
let mut headers = http::HeaderMap::new();
|
||||
auth.add_auth_headers(&mut headers);
|
||||
|
||||
let client = build_reqwest_client();
|
||||
client
|
||||
let client = build_reqwest_client(http_client_factory, url)?;
|
||||
Ok(client
|
||||
.request(method, url)
|
||||
.timeout(OPENAI_FILE_REQUEST_TIMEOUT)
|
||||
.headers(headers)
|
||||
.headers(headers))
|
||||
}
|
||||
|
||||
fn build_reqwest_client() -> reqwest::Client {
|
||||
build_reqwest_client_with_custom_ca(reqwest::Client::builder()).unwrap_or_else(|error| {
|
||||
tracing::warn!(error = %error, "failed to build OpenAI file upload client");
|
||||
reqwest::Client::new()
|
||||
})
|
||||
fn build_reqwest_client(
|
||||
http_client_factory: &HttpClientFactory,
|
||||
url: &str,
|
||||
) -> Result<reqwest::Client, OpenAiFileError> {
|
||||
match http_client_factory.build_reqwest_client(
|
||||
reqwest::Client::builder(),
|
||||
url,
|
||||
ClientRouteClass::Api,
|
||||
) {
|
||||
Ok(client) => Ok(client),
|
||||
Err(error)
|
||||
if matches!(
|
||||
http_client_factory.outbound_proxy_policy(),
|
||||
OutboundProxyPolicy::ReqwestDefault
|
||||
) =>
|
||||
{
|
||||
tracing::warn!(%error, "failed to build OpenAI file upload client");
|
||||
Ok(reqwest::Client::new())
|
||||
}
|
||||
Err(source) => Err(OpenAiFileError::ClientBuild {
|
||||
url: url.to_string(),
|
||||
source,
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
@@ -254,6 +294,10 @@ mod tests {
|
||||
#[derive(Clone, Copy)]
|
||||
struct ChatGptTestAuth;
|
||||
|
||||
fn default_http_client_factory() -> HttpClientFactory {
|
||||
HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault)
|
||||
}
|
||||
|
||||
impl AuthProvider for ChatGptTestAuth {
|
||||
fn add_auth_headers(&self, headers: &mut reqwest::header::HeaderMap) {
|
||||
headers.insert(
|
||||
@@ -324,6 +368,7 @@ mod tests {
|
||||
let uploaded = upload_openai_file(
|
||||
&base_url,
|
||||
&chatgpt_auth(),
|
||||
&default_http_client_factory(),
|
||||
"hello.txt".to_string(),
|
||||
/*file_size_bytes*/ 5,
|
||||
contents,
|
||||
|
||||
Reference in New Issue
Block a user