mirror of
https://github.com/openai/codex.git
synced 2026-10-19 16:17:49 +00:00
Avoid rendering generated images twice (#34378)
## What changed - Tell the model that generated images are already displayed to the user and should not be repeated as Markdown images or file links. - Keep generated-image artifact paths and output hints owned by the image generation extension. - Centralize the availability check for the image generation tool across feature, provider, namespace-tool, model modality, and authentication requirements. ## Testing - Verify artifact paths sanitize session and call identifiers. - Verify the model-facing output hint reports that the generated image is already displayed. GitOrigin-RevId: 6c865b6a96d6a71bb2c33246d1518a00e7c8b840
This commit is contained in:
@@ -165,6 +165,10 @@ async fn standalone_image_generation_returns_saved_path_hint_to_model() -> Resul
|
||||
output_hint.contains(&saved_path.display().to_string()),
|
||||
"output hint should identify the path the extension saved"
|
||||
);
|
||||
assert!(
|
||||
output_hint.contains("already displayed to the user"),
|
||||
"output hint should tell the model not to repeat the generated image: {output_hint}"
|
||||
);
|
||||
assert!(
|
||||
!requests[1]
|
||||
.message_input_texts("developer")
|
||||
|
||||
@@ -1,22 +0,0 @@
|
||||
use std::fmt::Display;
|
||||
|
||||
/// Maximum size of the extension's model-facing generated-image path hint.
|
||||
const MAX_IMAGE_GENERATION_OUTPUT_HINT_BYTES: usize = 1024;
|
||||
|
||||
/// Returns the extension's model-facing hint, or omits it if the path makes it too large.
|
||||
pub fn extension_image_generation_output_hint(
|
||||
image_output_dir: impl Display,
|
||||
image_output_path: impl Display,
|
||||
) -> Option<String> {
|
||||
let hint = image_generation_hint(image_output_dir, image_output_path);
|
||||
(hint.len() <= MAX_IMAGE_GENERATION_OUTPUT_HINT_BYTES).then_some(hint)
|
||||
}
|
||||
|
||||
fn image_generation_hint(
|
||||
image_output_dir: impl Display,
|
||||
image_output_path: impl Display,
|
||||
) -> String {
|
||||
format!(
|
||||
"Generated images are saved to {image_output_dir} as {image_output_path} by default.\nIf you need to use a generated image at another path, copy it and leave the original in place unless the user explicitly asks you to delete it."
|
||||
)
|
||||
}
|
||||
@@ -10,7 +10,6 @@ mod environment_context;
|
||||
mod environments_instructions;
|
||||
mod guardian_followup_review_reminder;
|
||||
mod hook_additional_context;
|
||||
mod image_generation_instructions;
|
||||
mod inter_agent_completion_message;
|
||||
mod internal_model_context;
|
||||
mod legacy_apply_patch_exec_command_warning;
|
||||
@@ -51,7 +50,6 @@ pub(crate) use current_time_reminder::CurrentTimeReminder;
|
||||
pub(crate) use environments_instructions::EnvironmentsInstructions;
|
||||
pub(crate) use guardian_followup_review_reminder::GuardianFollowupReviewReminder;
|
||||
pub(crate) use hook_additional_context::HookAdditionalContext;
|
||||
pub use image_generation_instructions::extension_image_generation_output_hint;
|
||||
pub(crate) use inter_agent_completion_message::InterAgentCompletionMessage;
|
||||
pub use internal_model_context::InternalContextSource;
|
||||
pub use internal_model_context::InternalModelContextFragment;
|
||||
|
||||
@@ -1047,7 +1047,6 @@ fn drop_last_n_user_turns_trims_context_updates_above_rolled_back_turn() {
|
||||
assistant_msg("session prefix item"),
|
||||
user_input_text_msg("turn 1 user"),
|
||||
assistant_msg("turn 1 assistant"),
|
||||
developer_msg("Generated images are saved to /tmp as /tmp/image-1.png by default."),
|
||||
developer_msg(&format!(
|
||||
"{APPS_INSTRUCTIONS_OPEN_TAG}\nROLLED_BACK_APPS_INSTRUCTIONS"
|
||||
)),
|
||||
@@ -1078,7 +1077,6 @@ fn drop_last_n_user_turns_trims_context_updates_above_rolled_back_turn() {
|
||||
assistant_msg("session prefix item"),
|
||||
user_input_text_msg("turn 1 user"),
|
||||
assistant_msg("turn 1 assistant"),
|
||||
developer_msg("Generated images are saved to /tmp as /tmp/image-1.png by default."),
|
||||
]
|
||||
);
|
||||
assert_eq!(
|
||||
|
||||
@@ -98,7 +98,6 @@ pub(crate) use skills::injection;
|
||||
pub(crate) use skills::maybe_emit_implicit_skill_invocation;
|
||||
pub(crate) use skills::skills_load_input_from_config;
|
||||
mod stream_events_utils;
|
||||
pub use stream_events_utils::image_generation_artifact_path;
|
||||
pub mod test_support;
|
||||
mod unified_exec;
|
||||
pub mod windows_sandbox;
|
||||
|
||||
@@ -25,44 +25,12 @@ use codex_protocol::models::MessagePhase;
|
||||
use codex_protocol::models::ResponseInputItem;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
use codex_rollout::state_db;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_stream_parser::strip_proposed_plan_blocks;
|
||||
use futures::Future;
|
||||
use tracing::debug;
|
||||
use tracing::instrument;
|
||||
use tracing::warn;
|
||||
|
||||
const GENERATED_IMAGE_ARTIFACTS_DIR: &str = "generated_images";
|
||||
|
||||
/// Returns the host-owned default artifact path for a generated image.
|
||||
pub fn image_generation_artifact_path(
|
||||
codex_home: &AbsolutePathBuf,
|
||||
session_id: &str,
|
||||
call_id: &str,
|
||||
) -> AbsolutePathBuf {
|
||||
let sanitize = |value: &str| {
|
||||
let mut sanitized: String = value
|
||||
.chars()
|
||||
.map(|ch| {
|
||||
if ch.is_ascii_alphanumeric() || ch == '-' || ch == '_' {
|
||||
ch
|
||||
} else {
|
||||
'_'
|
||||
}
|
||||
})
|
||||
.collect();
|
||||
if sanitized.is_empty() {
|
||||
sanitized = "generated_image".to_string();
|
||||
}
|
||||
sanitized
|
||||
};
|
||||
|
||||
codex_home
|
||||
.join(GENERATED_IMAGE_ARTIFACTS_DIR)
|
||||
.join(sanitize(session_id))
|
||||
.join(format!("{}.png", sanitize(call_id)))
|
||||
}
|
||||
|
||||
fn strip_hidden_assistant_markup(text: &str, plan_mode: bool) -> String {
|
||||
let (without_citations, _) = strip_citations(text);
|
||||
if plan_mode {
|
||||
|
||||
@@ -429,11 +429,6 @@ mod tests {
|
||||
async fn image_generation_publication_preserves_extension_saved_path() {
|
||||
let (session, turn, rx) = crate::session::tests::make_session_and_context_with_rx().await;
|
||||
let expected_path = test_path_buf("/tmp/extension-claimed.png").abs();
|
||||
let default_path = crate::stream_events_utils::image_generation_artifact_path(
|
||||
&turn.config.codex_home,
|
||||
&session.thread_id.to_string(),
|
||||
"call-image",
|
||||
);
|
||||
let emitter = CoreTurnItemEmitter {
|
||||
session: Arc::downgrade(&session),
|
||||
turn: Arc::downgrade(&turn),
|
||||
@@ -498,6 +493,5 @@ mod tests {
|
||||
|
||||
assert_eq!(started_item, expected_started_item);
|
||||
assert_eq!(completed_item, expected_completed_item);
|
||||
assert!(!default_path.exists());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -364,33 +364,36 @@ fn agent_jobs_worker_tools_enabled(turn_context: &TurnContext) -> bool {
|
||||
)
|
||||
}
|
||||
|
||||
fn image_generation_runtime_enabled(turn_context: &TurnContext) -> bool {
|
||||
(turn_context
|
||||
.provider
|
||||
.info()
|
||||
.uses_openai_actor_authorization()
|
||||
|| (turn_context.provider.info().requires_openai_auth
|
||||
&& turn_context
|
||||
.auth_manager
|
||||
.as_deref()
|
||||
.is_some_and(AuthManager::current_auth_uses_codex_backend)))
|
||||
&& turn_context.provider.capabilities().image_generation
|
||||
&& turn_context
|
||||
.model_info
|
||||
.input_modalities
|
||||
.contains(&InputModality::Image)
|
||||
}
|
||||
|
||||
fn standalone_image_generation_model_visible(turn_context: &TurnContext) -> bool {
|
||||
if !image_generation_runtime_enabled(turn_context) || !namespace_tools_enabled(turn_context) {
|
||||
return false;
|
||||
}
|
||||
|
||||
turn_context
|
||||
fn image_generation_available(turn_context: &TurnContext) -> bool {
|
||||
if !turn_context
|
||||
.config
|
||||
.features
|
||||
.get()
|
||||
.enabled(Feature::ImageGeneration)
|
||||
{
|
||||
return false;
|
||||
}
|
||||
|
||||
let capabilities = turn_context.provider.capabilities();
|
||||
if !capabilities.image_generation || !capabilities.namespace_tools {
|
||||
return false;
|
||||
}
|
||||
|
||||
if !turn_context
|
||||
.model_info
|
||||
.input_modalities
|
||||
.contains(&InputModality::Image)
|
||||
{
|
||||
return false;
|
||||
}
|
||||
|
||||
let provider = turn_context.provider.info();
|
||||
provider.uses_openai_actor_authorization()
|
||||
|| (provider.requires_openai_auth
|
||||
&& turn_context
|
||||
.auth_manager
|
||||
.as_deref()
|
||||
.is_some_and(AuthManager::current_auth_uses_codex_backend))
|
||||
}
|
||||
|
||||
fn wait_agent_timeout_options(turn_context: &TurnContext) -> WaitAgentTimeoutOptions {
|
||||
@@ -996,7 +999,7 @@ fn append_extension_tool_executors(
|
||||
continue;
|
||||
}
|
||||
if tool_name == ToolName::namespaced(IMAGE_GEN_NAMESPACE, IMAGEGEN_TOOL_NAME)
|
||||
&& !standalone_image_generation_model_visible(turn_context)
|
||||
&& !image_generation_available(turn_context)
|
||||
{
|
||||
continue;
|
||||
}
|
||||
|
||||
@@ -0,0 +1,46 @@
|
||||
use std::fmt::Display;
|
||||
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
|
||||
const GENERATED_IMAGE_ARTIFACTS_DIR: &str = "generated_images";
|
||||
const MAX_IMAGE_GENERATION_OUTPUT_HINT_BYTES: usize = 1024;
|
||||
|
||||
/// Returns the extension-owned artifact path for a generated image.
|
||||
pub(crate) fn image_generation_artifact_path(
|
||||
save_root: &AbsolutePathBuf,
|
||||
session_id: &str,
|
||||
call_id: &str,
|
||||
) -> AbsolutePathBuf {
|
||||
let sanitize = |value: &str| {
|
||||
let mut sanitized: String = value
|
||||
.chars()
|
||||
.map(|ch| {
|
||||
if ch.is_ascii_alphanumeric() || ch == '-' || ch == '_' {
|
||||
ch
|
||||
} else {
|
||||
'_'
|
||||
}
|
||||
})
|
||||
.collect();
|
||||
if sanitized.is_empty() {
|
||||
sanitized = "generated_image".to_string();
|
||||
}
|
||||
sanitized
|
||||
};
|
||||
|
||||
save_root
|
||||
.join(GENERATED_IMAGE_ARTIFACTS_DIR)
|
||||
.join(sanitize(session_id))
|
||||
.join(format!("{}.png", sanitize(call_id)))
|
||||
}
|
||||
|
||||
/// Returns the model-facing generated-image path hint, or omits it if it is too large.
|
||||
pub(crate) fn image_generation_output_hint(
|
||||
image_output_dir: impl Display,
|
||||
image_output_path: impl Display,
|
||||
) -> Option<String> {
|
||||
let hint = format!(
|
||||
"Generated images are saved to {image_output_dir} as {image_output_path} by default.\nIf you need to use a generated image at another path, copy it and leave the original in place unless the user explicitly asks you to delete it.\nThe generated image is already displayed to the user. There is no need to render it in the final response as a Markdown image or file link."
|
||||
);
|
||||
(hint.len() <= MAX_IMAGE_GENERATION_OUTPUT_HINT_BYTES).then_some(hint)
|
||||
}
|
||||
@@ -1,3 +1,4 @@
|
||||
mod artifact;
|
||||
mod backend;
|
||||
mod extension;
|
||||
mod tool;
|
||||
|
||||
@@ -3,7 +3,6 @@ use codex_api::ImageEditRequest;
|
||||
use codex_api::ImageGenerationRequest;
|
||||
use codex_api::ImageQuality;
|
||||
use codex_api::ImageUrl;
|
||||
use codex_core::context::extension_image_generation_output_hint;
|
||||
use codex_extension_api::ToolOutput;
|
||||
use codex_extension_api::ToolPayload;
|
||||
use codex_extension_api::ToolSpec;
|
||||
@@ -16,6 +15,7 @@ use codex_protocol::models::FunctionCallOutputPayload;
|
||||
use codex_protocol::models::ResponseInputItem;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
use codex_tools::ResponsesApiNamespaceTool;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
use super::GeneratedImageOutput;
|
||||
@@ -25,9 +25,24 @@ use super::imagegen_tool_spec;
|
||||
use super::request_for_call_args;
|
||||
use crate::IMAGE_GEN_NAMESPACE;
|
||||
use crate::IMAGEGEN_TOOL_NAME;
|
||||
use crate::artifact::image_generation_artifact_path;
|
||||
use crate::artifact::image_generation_output_hint;
|
||||
|
||||
const RESULT: &str = "cG5n";
|
||||
|
||||
#[test]
|
||||
fn artifact_path_sanitizes_session_and_call_ids() {
|
||||
let save_root = AbsolutePathBuf::current_dir().expect("current directory should be absolute");
|
||||
|
||||
assert_eq!(
|
||||
image_generation_artifact_path(&save_root, "../session", "../call"),
|
||||
save_root
|
||||
.join("generated_images")
|
||||
.join("___session")
|
||||
.join("___call.png")
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn uses_reserved_image_gen_namespace() {
|
||||
let ToolSpec::Namespace(spec) = imagegen_tool_spec() else {
|
||||
@@ -224,7 +239,7 @@ async fn recent_image_fallback_requires_requested_count() {
|
||||
#[test]
|
||||
fn generated_output_returns_image_input_and_output_hint() {
|
||||
let output_hint =
|
||||
extension_image_generation_output_hint("/tmp", "/tmp/call-1.png").expect("hint should fit");
|
||||
image_generation_output_hint("/tmp", "/tmp/call-1.png").expect("hint should fit");
|
||||
let output = GeneratedImageOutput {
|
||||
result: RESULT.to_string(),
|
||||
output_hint: Some(output_hint.clone()),
|
||||
@@ -273,7 +288,7 @@ fn generated_output_omits_oversized_output_hint() {
|
||||
let long_path = "x".repeat(1024);
|
||||
let output = GeneratedImageOutput {
|
||||
result: RESULT.to_string(),
|
||||
output_hint: extension_image_generation_output_hint("/tmp", long_path),
|
||||
output_hint: image_generation_output_hint("/tmp", long_path),
|
||||
};
|
||||
|
||||
let ResponseInputItem::FunctionCallOutput {
|
||||
|
||||
@@ -8,8 +8,6 @@ use codex_api::ImageEditRequest;
|
||||
use codex_api::ImageGenerationRequest;
|
||||
use codex_api::ImageQuality;
|
||||
use codex_api::ImageUrl;
|
||||
use codex_core::context::extension_image_generation_output_hint;
|
||||
use codex_core::image_generation_artifact_path;
|
||||
use codex_exec_server::CreateDirectoryOptions;
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
@@ -52,6 +50,8 @@ use serde_json::Value;
|
||||
|
||||
use crate::IMAGE_GEN_NAMESPACE;
|
||||
use crate::IMAGEGEN_TOOL_NAME;
|
||||
use crate::artifact::image_generation_artifact_path;
|
||||
use crate::artifact::image_generation_output_hint;
|
||||
use crate::backend::CodexImagesBackend;
|
||||
|
||||
const IMAGE_MODEL: &str = "gpt-image-2";
|
||||
@@ -217,7 +217,7 @@ impl ImageGenerationTool {
|
||||
.await;
|
||||
let output_hint = saved_path.as_ref().and_then(|output_path| {
|
||||
let output_dir = output_path.parent()?;
|
||||
extension_image_generation_output_hint(output_dir.display(), output_path.display())
|
||||
image_generation_output_hint(output_dir.display(), output_path.display())
|
||||
});
|
||||
Ok(Box::new(GeneratedImageOutput {
|
||||
result,
|
||||
|
||||
Reference in New Issue
Block a user