Fix js_repl view_image attachments in nested tool calls (#12725)

## Summary - Fix `js_repl` so `await codex.tool("view_image", { path })` actually attaches the image to the active turn when called from inside the JS REPL. - Restore the behavior expected by the existing `js_repl` image-attachment test. - This is a follow-up to [#12553](https://github.com/openai/codex/pull/12553), which changed `view_image` to return structured image content. ## Root Cause - [#12553](https://github.com/openai/codex/pull/12553) changed `view_image` from directly injecting a pending user image message to returning structured `function_call_output` content items. - The nested tool-call bridge inside `js_repl` serialized that tool response back to the JS runtime, but it did not mirror returned image content into the active turn. - As a result, `view_image` appeared to succeed inside `js_repl`, but no `input_image` was actually attached for the outer turn. ## What Changed - Updated the nested tool-call path in `js_repl` to inspect function tool responses for structured content items. - When a nested tool response includes `input_image` content, `js_repl` now injects a corresponding user `Message` into the active turn before returning the raw tool result back to the JS runtime. - Kept the normal JSON result flow intact, so `codex.tool(...)` still returns the original tool output object to JavaScript. ## Why - `js_repl` documentation and tests already assume that `view_image` can be used from inside the REPL to attach generated images to the model. - Without this fix, the nested call path silently dropped that attachment behavior.
2026-07-01 00:31:56 +08:00 · 2026-02-24 18:23:53 -08:00
parent 74e112ea09
commit 125fbec317
7 changed files with 223 additions and 76 deletions
@@ -6361,6 +6361,8 @@ use crate::memories::prompts::build_memory_tool_developer_instructions;
 #[cfg(test)]
 pub(crate) use tests::make_session_and_context;
 #[cfg(test)]
+pub(crate) use tests::make_session_and_context_with_dynamic_tools_and_rx;
+#[cfg(test)]
 pub(crate) use tests::make_session_and_context_with_rx;
 #[cfg(test)]
 pub(crate) use tests::make_session_configuration_for_tests;
@@ -8273,9 +8275,9 @@ mod tests {
        (session, turn_context)
    }

-    // Like make_session_and_context, but returns Arc<Session> and the event receiver
-    // so tests can assert on emitted events.
-    pub(crate) async fn make_session_and_context_with_rx() -> (
+    pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx(
+        dynamic_tools: Vec<DynamicToolSpec>,
+    ) -> (
        Arc<Session>,
        Arc<TurnContext>,
        async_channel::Receiver<Event>,
@@ -8327,7 +8329,7 @@ mod tests {
            thread_name: None,
            original_config_do_not_use: Arc::clone(&config),
            session_source: SessionSource::Exec,
-            dynamic_tools: Vec::new(),
+            dynamic_tools,
            persist_extended_history: false,
        };
        let per_turn_config = Session::build_per_turn_config(&session_configuration);
@@ -8429,6 +8431,16 @@ mod tests {
        (session, turn_context, rx_event)
    }

+    // Like make_session_and_context, but returns Arc<Session> and the event receiver
+    // so tests can assert on emitted events.
+    pub(crate) async fn make_session_and_context_with_rx() -> (
+        Arc<Session>,
+        Arc<TurnContext>,
+        async_channel::Receiver<Event>,
+    ) {
+        make_session_and_context_with_dynamic_tools_and_rx(Vec::new()).await
+    }
+
    #[tokio::test]
    async fn refresh_mcp_servers_is_deferred_until_next_turn() {
        let (session, turn_context) = make_session_and_context().await;
@@ -9273,7 +9285,6 @@ mod tests {
                    })
                    .to_string(),
                },
-                source: ToolCallSource::Direct,
            })
            .await;

@@ -9313,7 +9324,6 @@ mod tests {
                    })
                    .to_string(),
                },
-                source: ToolCallSource::Direct,
            })
            .await;

@@ -9373,7 +9383,6 @@ mod tests {
                    })
                    .to_string(),
                },
-                source: ToolCallSource::Direct,
            })
            .await;

@@ -30,7 +30,6 @@ pub struct ToolInvocation {
    pub call_id: String,
    pub tool_name: String,
    pub payload: ToolPayload,
-    pub source: ToolCallSource,
 }

 #[derive(Clone, Debug)]
@@ -1004,7 +1004,6 @@ mod tests {
            call_id: "call-1".to_string(),
            tool_name: tool_name.to_string(),
            payload,
-            source: crate::tools::router::ToolCallSource::Direct,
        }
    }

@@ -8,7 +8,6 @@ use tokio::fs;
 use crate::function_tool::FunctionCallError;
 use crate::protocol::EventMsg;
 use crate::protocol::ViewImageToolCallEvent;
-use crate::tools::context::ToolCallSource;
 use crate::tools::context::ToolInvocation;
 use crate::tools::context::ToolOutput;
 use crate::tools::context::ToolPayload;
@@ -16,7 +15,6 @@ use crate::tools::handlers::parse_arguments;
 use crate::tools::registry::ToolHandler;
 use crate::tools::registry::ToolKind;
 use codex_protocol::models::ContentItem;
-use codex_protocol::models::ResponseInputItem;
 use codex_protocol::models::local_image_content_items_with_label_number;

 pub struct ViewImageHandler;
@@ -52,7 +50,6 @@ impl ToolHandler for ViewImageHandler {
            turn,
            payload,
            call_id,
-            source,
            ..
        } = invocation;

@@ -85,24 +82,6 @@ impl ToolHandler for ViewImageHandler {
        let event_path = abs_path.clone();

        let content = local_image_content_items_with_label_number(&abs_path, None);
-        if source == ToolCallSource::JsRepl
-            && content
-                .iter()
-                .any(|item| matches!(item, ContentItem::InputImage { .. }))
-        {
-            let input_item = ResponseInputItem::Message {
-                role: "user".to_string(),
-                content: content.clone(),
-            };
-            if session
-                .inject_response_items(vec![input_item])
-                .await
-                .is_err()
-            {
-                tracing::warn!("view_image could not find an active turn to attach image input");
-            }
-        }
-
        let content = content
            .into_iter()
            .map(|item| match item {
@@ -9,6 +9,9 @@ use std::sync::Arc;
 use std::time::Duration;

 use codex_protocol::ThreadId;
+use codex_protocol::models::ContentItem;
+use codex_protocol::models::FunctionCallOutputContentItem;
+use codex_protocol::models::ResponseInputItem;
 use serde::Deserialize;
 use serde::Serialize;
 use serde_json::Value as JsonValue;
@@ -1050,36 +1053,78 @@ impl JsReplManager {
                }
            };

+        let tool_name = req.tool_name.clone();
        let call = crate::tools::router::ToolCall {
-            tool_name: req.tool_name,
+            tool_name: tool_name.clone(),
            call_id: req.id.clone(),
            payload,
        };

+        let session = Arc::clone(&exec.session);
+        let turn = Arc::clone(&exec.turn);
+        let tracker = Arc::clone(&exec.tracker);
+
        match router
            .dispatch_tool_call(
-                exec.session,
-                exec.turn,
-                exec.tracker,
+                session.clone(),
+                turn,
+                tracker,
                call,
                crate::tools::router::ToolCallSource::JsRepl,
            )
            .await
        {
-            Ok(response) => match serde_json::to_value(response) {
-                Ok(value) => RunToolResult {
-                    id: req.id,
-                    ok: true,
-                    response: Some(value),
-                    error: None,
-                },
-                Err(err) => RunToolResult {
-                    id: req.id,
-                    ok: false,
-                    response: None,
-                    error: Some(format!("failed to serialize tool output: {err}")),
-                },
-            },
+            Ok(response) => {
+                if let ResponseInputItem::FunctionCallOutput { output, .. } = &response
+                    && let Some(items) = output.content_items()
+                {
+                    let mut has_image = false;
+                    let mut content = Vec::with_capacity(items.len());
+                    for item in items {
+                        match item {
+                            FunctionCallOutputContentItem::InputText { text } => {
+                                content.push(ContentItem::InputText { text: text.clone() });
+                            }
+                            FunctionCallOutputContentItem::InputImage { image_url } => {
+                                has_image = true;
+                                content.push(ContentItem::InputImage {
+                                    image_url: image_url.clone(),
+                                });
+                            }
+                        }
+                    }
+
+                    if has_image
+                        && session
+                            .inject_response_items(vec![ResponseInputItem::Message {
+                                role: "user".to_string(),
+                                content,
+                            }])
+                            .await
+                            .is_err()
+                    {
+                        warn!(
+                            tool_name = %tool_name,
+                            "js_repl tool call returned image content but there was no active turn to attach it to"
+                        );
+                    }
+                }
+
+                match serde_json::to_value(response) {
+                    Ok(value) => RunToolResult {
+                        id: req.id,
+                        ok: true,
+                        response: Some(value),
+                        error: None,
+                    },
+                    Err(err) => RunToolResult {
+                        id: req.id,
+                        ok: false,
+                        response: None,
+                        error: Some(format!("failed to serialize tool output: {err}")),
+                    },
+                }
+            }
            Err(err) => RunToolResult {
                id: req.id,
                ok: false,
@@ -1301,9 +1346,14 @@ pub(crate) fn resolve_node(config_path: Option<&Path>) -> Option<PathBuf> {
 mod tests {
    use super::*;
    use crate::codex::make_session_and_context;
+    use crate::codex::make_session_and_context_with_dynamic_tools_and_rx;
    use crate::protocol::AskForApproval;
+    use crate::protocol::EventMsg;
    use crate::protocol::SandboxPolicy;
    use crate::turn_diff_tracker::TurnDiffTracker;
+    use codex_protocol::dynamic_tools::DynamicToolCallOutputContentItem;
+    use codex_protocol::dynamic_tools::DynamicToolResponse;
+    use codex_protocol::dynamic_tools::DynamicToolSpec;
    use codex_protocol::models::ContentItem;
    use codex_protocol::models::ResponseInputItem;
    use codex_protocol::openai_models::InputModality;
@@ -2116,23 +2166,118 @@ console.log(out.output?.body?.text ?? "");
        assert!(result.output.contains("function_call_output"));

        let pending_input = session.get_pending_input().await;
-        let image_url = pending_input
-            .iter()
-            .find_map(|item| match item {
-                ResponseInputItem::Message { content, .. } => {
-                    content.iter().find_map(|content_item| match content_item {
-                        ContentItem::InputImage { image_url } => Some(image_url.as_str()),
-                        _ => None,
-                    })
-                }
-                _ => None,
-            })
-            .expect("view_image should inject an input_image message for the active turn");
+        let [ResponseInputItem::Message { role, content }] = pending_input.as_slice() else {
+            panic!(
+                "view_image should inject exactly one pending input message, got {pending_input:?}"
+            );
+        };
+        assert_eq!(role, "user");
+        let [ContentItem::InputImage { image_url }] = content.as_slice() else {
+            panic!(
+                "view_image should inject exactly one input_image content item, got {content:?}"
+            );
+        };
        assert!(image_url.starts_with("data:image/png;base64,"));

        Ok(())
    }

+    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+    async fn js_repl_can_attach_image_via_dynamic_tool_with_mixed_content() -> anyhow::Result<()> {
+        if !can_run_js_repl_runtime_tests().await {
+            return Ok(());
+        }
+
+        let (session, turn, rx_event) =
+            make_session_and_context_with_dynamic_tools_and_rx(vec![DynamicToolSpec {
+                name: "inline_image".to_string(),
+                description: "Returns inline text and image content.".to_string(),
+                input_schema: serde_json::json!({
+                    "type": "object",
+                    "properties": {},
+                    "additionalProperties": false
+                }),
+            }])
+            .await;
+        if !turn
+            .model_info
+            .input_modalities
+            .contains(&InputModality::Image)
+        {
+            return Ok(());
+        }
+
+        *session.active_turn.lock().await = Some(crate::state::ActiveTurn::default());
+
+        let tracker = Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::default()));
+        let manager = turn.js_repl.manager().await?;
+        let code = r#"
+const out = await codex.tool("inline_image", {});
+console.log(out.type);
+"#;
+        let image_url = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR4nGP4z8DwHwAFAAH/iZk9HQAAAABJRU5ErkJggg==";
+
+        let session_for_response = Arc::clone(&session);
+        let response_watcher = async move {
+            loop {
+                let event = tokio::time::timeout(Duration::from_secs(2), rx_event.recv()).await??;
+                if let EventMsg::DynamicToolCallRequest(request) = event.msg {
+                    session_for_response
+                        .notify_dynamic_tool_response(
+                            &request.call_id,
+                            DynamicToolResponse {
+                                content_items: vec![
+                                    DynamicToolCallOutputContentItem::InputText {
+                                        text: "inline image note".to_string(),
+                                    },
+                                    DynamicToolCallOutputContentItem::InputImage {
+                                        image_url: image_url.to_string(),
+                                    },
+                                ],
+                                success: true,
+                            },
+                        )
+                        .await;
+                    return Ok::<(), anyhow::Error>(());
+                }
+            }
+        };
+
+        let (result, response_watcher_result) = tokio::join!(
+            manager.execute(
+                Arc::clone(&session),
+                Arc::clone(&turn),
+                tracker,
+                JsReplArgs {
+                    code: code.to_string(),
+                    timeout_ms: Some(15_000),
+                },
+            ),
+            response_watcher,
+        );
+        response_watcher_result?;
+        let result = result?;
+        assert!(result.output.contains("function_call_output"));
+
+        let pending_input = session.get_pending_input().await;
+        assert_eq!(
+            pending_input,
+            vec![ResponseInputItem::Message {
+                role: "user".to_string(),
+                content: vec![
+                    ContentItem::InputText {
+                        text: "inline image note".to_string(),
+                    },
+                    ContentItem::InputImage {
+                        image_url: image_url.to_string(),
+                    },
+                ],
+            }]
+        );
+
+        Ok(())
+    }
+
    #[tokio::test]
    async fn js_repl_does_not_expose_process_global() -> anyhow::Result<()> {
        if !can_run_js_repl_runtime_tests().await {
@@ -175,7 +175,6 @@ impl ToolRouter {
            call_id,
            tool_name,
            payload,
-            source,
        };

        match self.registry.dispatch(invocation).await {
@@ -36,28 +36,37 @@ use image::GenericImageView;
 use image::ImageBuffer;
 use image::Rgba;
 use image::load_from_memory;
+use pretty_assertions::assert_eq;
 use serde_json::Value;
 use tokio::time::Duration;
 use wiremock::BodyPrintLimit;
 use wiremock::MockServer;

-fn find_image_message(body: &Value) -> Option<&Value> {
+fn image_messages(body: &Value) -> Vec<&Value> {
    body.get("input")
        .and_then(Value::as_array)
-        .and_then(|items| {
-            items.iter().find(|item| {
-                item.get("type").and_then(Value::as_str) == Some("message")
-                    && item
-                        .get("content")
-                        .and_then(Value::as_array)
-                        .map(|content| {
-                            content.iter().any(|span| {
-                                span.get("type").and_then(Value::as_str) == Some("input_image")
+        .map(|items| {
+            items
+                .iter()
+                .filter(|item| {
+                    item.get("type").and_then(Value::as_str) == Some("message")
+                        && item
+                            .get("content")
+                            .and_then(Value::as_array)
+                            .map(|content| {
+                                content.iter().any(|span| {
+                                    span.get("type").and_then(Value::as_str) == Some("input_image")
+                                })
                            })
-                        })
-                        .unwrap_or(false)
-            })
+                            .unwrap_or(false)
+                })
+                .collect()
        })
+        .unwrap_or_default()
+}
+
+fn find_image_message(body: &Value) -> Option<&Value> {
+    image_messages(body).into_iter().next()
 }

 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
@@ -366,8 +375,16 @@ console.log(out.output?.body?.text ?? "");
    );

    let body = req.body_json();
-    let image_message =
-        find_image_message(&body).expect("pending input image message not included in request");
+    let image_messages = image_messages(&body);
+    assert_eq!(
+        image_messages.len(),
+        1,
+        "js_repl view_image should inject exactly one pending input image message"
+    );
+    let image_message = image_messages
+        .into_iter()
+        .next()
+        .expect("pending input image message not included in request");
    let image_url = image_message
        .get("content")
        .and_then(Value::as_array)