mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Let models opt into original image detail (#14175)
## Summary This PR narrows original image detail handling to a single opt-in feature: - `image_detail_original` lets the model request `detail: "original"` on supported models - Omitting `detail` preserves the default resized behavior The model only sees `detail: "original"` guidance when the active model supports it: - JS REPL instructions include the guidance and examples only on supported models - `view_image` only exposes a `detail` parameter when the feature and model can use it The image detail API is intentionally narrow and consistent across both paths: - `view_image.detail` supports only `"original"`; otherwise omit the field - `codex.emitImage(..., detail)` supports only `"original"`; otherwise omit the field - Unsupported explicit values fail clearly at the API boundary instead of being silently reinterpreted - Unsupported explicit `detail: "original"` requests fall back to normal behavior when the feature is disabled or the model does not support original detail
This commit is contained in:
committed by
GitHub
Unverified
parent
f548309797
commit
8791f0ab9a
@@ -8,8 +8,8 @@ use codex_utils_image::PromptImageMode;
|
||||
use serde::Deserialize;
|
||||
use tokio::fs;
|
||||
|
||||
use crate::features::Feature;
|
||||
use crate::function_tool::FunctionCallError;
|
||||
use crate::original_image_detail::can_request_original_image_detail;
|
||||
use crate::protocol::EventMsg;
|
||||
use crate::protocol::ViewImageToolCallEvent;
|
||||
use crate::tools::context::FunctionToolOutput;
|
||||
@@ -27,6 +27,12 @@ const VIEW_IMAGE_UNSUPPORTED_MESSAGE: &str =
|
||||
#[derive(Deserialize)]
|
||||
struct ViewImageArgs {
|
||||
path: String,
|
||||
detail: Option<String>,
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy, Eq, PartialEq)]
|
||||
enum ViewImageDetail {
|
||||
Original,
|
||||
}
|
||||
|
||||
#[async_trait]
|
||||
@@ -67,6 +73,19 @@ impl ToolHandler for ViewImageHandler {
|
||||
};
|
||||
|
||||
let args: ViewImageArgs = parse_arguments(&arguments)?;
|
||||
// `view_image` accepts only its documented detail values: omit
|
||||
// `detail` for the default path or set it to `original`.
|
||||
// Other string values remain invalid rather than being silently
|
||||
// reinterpreted.
|
||||
let detail = match args.detail.as_deref() {
|
||||
None => None,
|
||||
Some("original") => Some(ViewImageDetail::Original),
|
||||
Some(detail) => {
|
||||
return Err(FunctionCallError::RespondToModel(format!(
|
||||
"view_image.detail only supports `original`; omit `detail` for default resized behavior, got `{detail}`"
|
||||
)));
|
||||
}
|
||||
};
|
||||
|
||||
let abs_path = turn.resolve_path(Some(args.path));
|
||||
|
||||
@@ -85,8 +104,10 @@ impl ToolHandler for ViewImageHandler {
|
||||
}
|
||||
let event_path = abs_path.clone();
|
||||
|
||||
let use_original_detail = turn.config.features.enabled(Feature::ImageDetailOriginal)
|
||||
&& turn.model_info.supports_image_detail_original;
|
||||
let can_request_original_detail =
|
||||
can_request_original_image_detail(turn.features.get(), &turn.model_info);
|
||||
let use_original_detail =
|
||||
can_request_original_detail && matches!(detail, Some(ViewImageDetail::Original));
|
||||
let image_mode = if use_original_detail {
|
||||
PromptImageMode::Original
|
||||
} else {
|
||||
|
||||
@@ -1210,20 +1210,15 @@ function encodeByteImage(bytes, mimeType, detail) {
|
||||
}
|
||||
|
||||
function parseImageDetail(detail) {
|
||||
if (typeof detail === "undefined") {
|
||||
if (detail == null) {
|
||||
return undefined;
|
||||
}
|
||||
if (typeof detail !== "string" || !detail) {
|
||||
throw new Error("codex.emitImage expected detail to be a non-empty string");
|
||||
}
|
||||
if (
|
||||
detail !== "auto" &&
|
||||
detail !== "low" &&
|
||||
detail !== "high" &&
|
||||
detail !== "original"
|
||||
) {
|
||||
if (detail !== "original") {
|
||||
throw new Error(
|
||||
'codex.emitImage expected detail to be one of "auto", "low", "high", or "original"',
|
||||
'codex.emitImage only supports detail "original"; omit detail for default behavior',
|
||||
);
|
||||
}
|
||||
return detail;
|
||||
|
||||
@@ -36,8 +36,8 @@ use crate::codex::Session;
|
||||
use crate::codex::TurnContext;
|
||||
use crate::exec::ExecExpiration;
|
||||
use crate::exec_env::create_env;
|
||||
use crate::features::Feature;
|
||||
use crate::function_tool::FunctionCallError;
|
||||
use crate::original_image_detail::normalize_output_image_detail;
|
||||
use crate::sandboxing::CommandSpec;
|
||||
use crate::sandboxing::SandboxManager;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
@@ -1478,7 +1478,7 @@ fn emitted_image_content_item(
|
||||
) -> FunctionCallOutputContentItem {
|
||||
FunctionCallOutputContentItem::InputImage {
|
||||
image_url,
|
||||
detail: detail.or_else(|| default_output_image_detail_for_turn(turn)),
|
||||
detail: normalize_output_image_detail(turn.features.get(), &turn.model_info, detail),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1493,12 +1493,6 @@ fn validate_emitted_image_url(image_url: &str) -> Result<(), String> {
|
||||
}
|
||||
}
|
||||
|
||||
fn default_output_image_detail_for_turn(turn: &TurnContext) -> Option<ImageDetail> {
|
||||
(turn.config.features.enabled(Feature::ImageDetailOriginal)
|
||||
&& turn.model_info.supports_image_detail_original)
|
||||
.then_some(ImageDetail::Original)
|
||||
}
|
||||
|
||||
fn build_exec_result_content_items(
|
||||
output: String,
|
||||
content_items: Vec<FunctionCallOutputContentItem>,
|
||||
@@ -2004,7 +1998,7 @@ mod tests {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn emitted_image_content_item_preserves_explicit_detail() {
|
||||
async fn emitted_image_content_item_drops_unsupported_explicit_detail() {
|
||||
let (_session, turn) = make_session_and_context().await;
|
||||
let content_item = emitted_image_content_item(
|
||||
&turn,
|
||||
@@ -2015,23 +2009,53 @@ mod tests {
|
||||
content_item,
|
||||
FunctionCallOutputContentItem::InputImage {
|
||||
image_url: "data:image/png;base64,AAA".to_string(),
|
||||
detail: Some(ImageDetail::Low),
|
||||
detail: None,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn emitted_image_content_item_uses_turn_original_detail_when_enabled() {
|
||||
async fn emitted_image_content_item_does_not_force_original_when_enabled() {
|
||||
let (_session, mut turn) = make_session_and_context().await;
|
||||
Arc::make_mut(&mut turn.config)
|
||||
.features
|
||||
.enable(Feature::ImageDetailOriginal)
|
||||
.expect("test config should allow feature update");
|
||||
turn.features
|
||||
.enable(Feature::ImageDetailOriginal)
|
||||
.expect("test turn features should allow feature update");
|
||||
turn.model_info.supports_image_detail_original = true;
|
||||
|
||||
let content_item =
|
||||
emitted_image_content_item(&turn, "data:image/png;base64,AAA".to_string(), None);
|
||||
|
||||
assert_eq!(
|
||||
content_item,
|
||||
FunctionCallOutputContentItem::InputImage {
|
||||
image_url: "data:image/png;base64,AAA".to_string(),
|
||||
detail: None,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn emitted_image_content_item_allows_explicit_original_detail_when_enabled() {
|
||||
let (_session, mut turn) = make_session_and_context().await;
|
||||
Arc::make_mut(&mut turn.config)
|
||||
.features
|
||||
.enable(Feature::ImageDetailOriginal)
|
||||
.expect("test config should allow feature update");
|
||||
turn.features
|
||||
.enable(Feature::ImageDetailOriginal)
|
||||
.expect("test turn features should allow feature update");
|
||||
turn.model_info.supports_image_detail_original = true;
|
||||
|
||||
let content_item = emitted_image_content_item(
|
||||
&turn,
|
||||
"data:image/png;base64,AAA".to_string(),
|
||||
Some(ImageDetail::Original),
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
content_item,
|
||||
FunctionCallOutputContentItem::InputImage {
|
||||
@@ -2041,6 +2065,25 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn emitted_image_content_item_drops_explicit_original_detail_when_disabled() {
|
||||
let (_session, turn) = make_session_and_context().await;
|
||||
|
||||
let content_item = emitted_image_content_item(
|
||||
&turn,
|
||||
"data:image/png;base64,AAA".to_string(),
|
||||
Some(ImageDetail::Original),
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
content_item,
|
||||
FunctionCallOutputContentItem::InputImage {
|
||||
image_url: "data:image/png;base64,AAA".to_string(),
|
||||
detail: None,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn validate_emitted_image_url_accepts_case_insensitive_data_scheme() {
|
||||
assert_eq!(
|
||||
@@ -3084,7 +3127,63 @@ await codex.emitImage({ bytes: png, mimeType: "image/png", detail: "ultra" });
|
||||
)
|
||||
.await
|
||||
.expect_err("invalid detail should fail");
|
||||
assert!(err.to_string().contains("expected detail to be one of"));
|
||||
assert!(
|
||||
err.to_string()
|
||||
.contains("only supports detail \"original\"")
|
||||
);
|
||||
assert!(session.get_pending_input().await.is_empty());
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn js_repl_emit_image_treats_null_detail_as_omitted() -> anyhow::Result<()> {
|
||||
if !can_run_js_repl_runtime_tests().await {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let (session, turn) = make_session_and_context().await;
|
||||
if !turn
|
||||
.model_info
|
||||
.input_modalities
|
||||
.contains(&InputModality::Image)
|
||||
{
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let session = Arc::new(session);
|
||||
let turn = Arc::new(turn);
|
||||
*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 png = Buffer.from(
|
||||
"iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR4nGP4z8DwHwAFAAH/iZk9HQAAAABJRU5ErkJggg==",
|
||||
"base64"
|
||||
);
|
||||
await codex.emitImage({ bytes: png, mimeType: "image/png", detail: null });
|
||||
"#;
|
||||
|
||||
let result = manager
|
||||
.execute(
|
||||
Arc::clone(&session),
|
||||
turn,
|
||||
tracker,
|
||||
JsReplArgs {
|
||||
code: code.to_string(),
|
||||
timeout_ms: Some(15_000),
|
||||
},
|
||||
)
|
||||
.await?;
|
||||
assert_eq!(
|
||||
result.content_items.as_slice(),
|
||||
[FunctionCallOutputContentItem::InputImage {
|
||||
image_url: "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR4nGP4z8DwHwAFAAH/iZk9HQAAAABJRU5ErkJggg==".to_string(),
|
||||
detail: None,
|
||||
}]
|
||||
.as_slice()
|
||||
);
|
||||
assert!(session.get_pending_input().await.is_empty());
|
||||
|
||||
Ok(())
|
||||
|
||||
@@ -7,6 +7,7 @@ use crate::features::Feature;
|
||||
use crate::features::Features;
|
||||
use crate::mcp_connection_manager::ToolInfo;
|
||||
use crate::models_manager::collaboration_mode_presets::CollaborationModesConfig;
|
||||
use crate::original_image_detail::can_request_original_image_detail;
|
||||
use crate::tools::code_mode::PUBLIC_TOOL_NAME;
|
||||
use crate::tools::code_mode_description::augment_tool_spec_for_code_mode;
|
||||
use crate::tools::handlers::PLAN_TOOL;
|
||||
@@ -108,6 +109,7 @@ pub(crate) struct ToolsConfig {
|
||||
pub code_mode_enabled: bool,
|
||||
pub js_repl_enabled: bool,
|
||||
pub js_repl_tools_only: bool,
|
||||
pub can_request_original_image_detail: bool,
|
||||
pub collab_tools: bool,
|
||||
pub artifact_tools: bool,
|
||||
pub request_user_input: bool,
|
||||
@@ -145,6 +147,7 @@ impl ToolsConfig {
|
||||
let include_default_mode_request_user_input =
|
||||
include_request_user_input && features.enabled(Feature::DefaultModeRequestUserInput);
|
||||
let include_search_tool = features.enabled(Feature::Apps);
|
||||
let include_original_image_detail = can_request_original_image_detail(features, model_info);
|
||||
let include_artifact_tools =
|
||||
features.enabled(Feature::Artifact) && codex_artifacts::can_manage_artifact_runtime();
|
||||
let include_image_gen_tool =
|
||||
@@ -216,6 +219,7 @@ impl ToolsConfig {
|
||||
code_mode_enabled: include_code_mode,
|
||||
js_repl_enabled: include_js_repl,
|
||||
js_repl_tools_only: include_js_repl_tools_only,
|
||||
can_request_original_image_detail: include_original_image_detail,
|
||||
collab_tools: include_collab_tools,
|
||||
artifact_tools: include_artifact_tools,
|
||||
request_user_input: include_request_user_input,
|
||||
@@ -694,14 +698,24 @@ Examples of valid command strings:
|
||||
})
|
||||
}
|
||||
|
||||
fn create_view_image_tool() -> ToolSpec {
|
||||
fn create_view_image_tool(can_request_original_image_detail: bool) -> ToolSpec {
|
||||
// Support only local filesystem path.
|
||||
let properties = BTreeMap::from([(
|
||||
let mut properties = BTreeMap::from([(
|
||||
"path".to_string(),
|
||||
JsonSchema::String {
|
||||
description: Some("Local filesystem path to an image file".to_string()),
|
||||
},
|
||||
)]);
|
||||
if can_request_original_image_detail {
|
||||
properties.insert(
|
||||
"detail".to_string(),
|
||||
JsonSchema::String {
|
||||
description: Some(
|
||||
"Optional detail override. The only supported value is `original`; omit this field for default resized behavior. Use `original` to preserve the file's original resolution instead of resizing to fit. This is important when high-fidelity image perception or precise localization is needed, especially for CUA agents.".to_string(),
|
||||
),
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
ToolSpec::Function(ResponsesApiTool {
|
||||
name: VIEW_IMAGE_TOOL_NAME.to_string(),
|
||||
@@ -2366,7 +2380,7 @@ pub(crate) fn build_specs(
|
||||
|
||||
push_tool_spec(
|
||||
&mut builder,
|
||||
create_view_image_tool(),
|
||||
create_view_image_tool(config.can_request_original_image_detail),
|
||||
true,
|
||||
config.code_mode_enabled,
|
||||
);
|
||||
@@ -2813,7 +2827,7 @@ mod tests {
|
||||
search_context_size: None,
|
||||
search_content_types: None,
|
||||
},
|
||||
create_view_image_tool(),
|
||||
create_view_image_tool(config.can_request_original_image_detail),
|
||||
] {
|
||||
expected.insert(tool_name(&spec).to_string(), spec);
|
||||
}
|
||||
@@ -2890,6 +2904,67 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn view_image_tool_omits_detail_without_original_detail_feature() {
|
||||
let config = test_config();
|
||||
let mut model_info =
|
||||
ModelsManager::construct_model_info_offline_for_tests("gpt-5-codex", &config);
|
||||
model_info.supports_image_detail_original = true;
|
||||
let features = Features::with_defaults();
|
||||
let available_models = Vec::new();
|
||||
let tools_config = ToolsConfig::new(&ToolsConfigParams {
|
||||
model_info: &model_info,
|
||||
available_models: &available_models,
|
||||
features: &features,
|
||||
web_search_mode: Some(WebSearchMode::Cached),
|
||||
session_source: SessionSource::Cli,
|
||||
});
|
||||
let (tools, _) = build_specs(&tools_config, None, None, &[]).build();
|
||||
let view_image = find_tool(&tools, VIEW_IMAGE_TOOL_NAME);
|
||||
let ToolSpec::Function(ResponsesApiTool { parameters, .. }) = &view_image.spec else {
|
||||
panic!("view_image should be a function tool");
|
||||
};
|
||||
let JsonSchema::Object { properties, .. } = parameters else {
|
||||
panic!("view_image should use an object schema");
|
||||
};
|
||||
assert!(!properties.contains_key("detail"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn view_image_tool_includes_detail_with_original_detail_feature() {
|
||||
let config = test_config();
|
||||
let mut model_info =
|
||||
ModelsManager::construct_model_info_offline_for_tests("gpt-5-codex", &config);
|
||||
model_info.supports_image_detail_original = true;
|
||||
let mut features = Features::with_defaults();
|
||||
features.enable(Feature::ImageDetailOriginal);
|
||||
let available_models = Vec::new();
|
||||
let tools_config = ToolsConfig::new(&ToolsConfigParams {
|
||||
model_info: &model_info,
|
||||
available_models: &available_models,
|
||||
features: &features,
|
||||
web_search_mode: Some(WebSearchMode::Cached),
|
||||
session_source: SessionSource::Cli,
|
||||
});
|
||||
let (tools, _) = build_specs(&tools_config, None, None, &[]).build();
|
||||
let view_image = find_tool(&tools, VIEW_IMAGE_TOOL_NAME);
|
||||
let ToolSpec::Function(ResponsesApiTool { parameters, .. }) = &view_image.spec else {
|
||||
panic!("view_image should be a function tool");
|
||||
};
|
||||
let JsonSchema::Object { properties, .. } = parameters else {
|
||||
panic!("view_image should use an object schema");
|
||||
};
|
||||
assert!(properties.contains_key("detail"));
|
||||
let Some(JsonSchema::String {
|
||||
description: Some(description),
|
||||
}) = properties.get("detail")
|
||||
else {
|
||||
panic!("view_image detail should include a description");
|
||||
};
|
||||
assert!(description.contains("only supported value is `original`"));
|
||||
assert!(description.contains("omit this field for default resized behavior"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_build_specs_artifact_tool_enabled() {
|
||||
let mut config = test_config();
|
||||
|
||||
Reference in New Issue
Block a user