mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
core: resolve view_image paths in selected environment (#29526)
## Why view_image needs to support foreign OS remote executors. ## What - resolve image paths against the selected environment as `PathUri` and read them through that environment's filesystem - keep app-server's public path field wire-compatible as `LegacyAppPathString`, with purpose-specific UI rendering - cover relative and absolute target-native paths in the core integration test and run the full `view_image` suite under wine-exec without skips
This commit is contained in:
committed by
GitHub
Unverified
parent
c660e2b644
commit
510bce9927
@@ -4444,7 +4444,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"path": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"type": {
|
||||
"enum": [
|
||||
|
||||
+1
-1
@@ -18038,7 +18038,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"path": {
|
||||
"$ref": "#/definitions/v2/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/v2/LegacyAppPathString"
|
||||
},
|
||||
"type": {
|
||||
"enum": [
|
||||
|
||||
+1
-1
@@ -15817,7 +15817,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"path": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"type": {
|
||||
"enum": [
|
||||
|
||||
+1
-1
@@ -1106,7 +1106,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"path": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"type": {
|
||||
"enum": [
|
||||
|
||||
@@ -1106,7 +1106,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"path": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"type": {
|
||||
"enum": [
|
||||
|
||||
@@ -1250,7 +1250,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"path": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"type": {
|
||||
"enum": [
|
||||
|
||||
@@ -1755,7 +1755,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"path": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"type": {
|
||||
"enum": [
|
||||
|
||||
@@ -1562,7 +1562,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"path": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"type": {
|
||||
"enum": [
|
||||
|
||||
+1
-1
@@ -1562,7 +1562,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"path": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"type": {
|
||||
"enum": [
|
||||
|
||||
@@ -1562,7 +1562,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"path": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"type": {
|
||||
"enum": [
|
||||
|
||||
@@ -1755,7 +1755,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"path": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"type": {
|
||||
"enum": [
|
||||
|
||||
@@ -1562,7 +1562,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"path": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"type": {
|
||||
"enum": [
|
||||
|
||||
@@ -1755,7 +1755,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"path": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"type": {
|
||||
"enum": [
|
||||
|
||||
+1
-1
@@ -1562,7 +1562,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"path": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"type": {
|
||||
"enum": [
|
||||
|
||||
@@ -1562,7 +1562,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"path": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"type": {
|
||||
"enum": [
|
||||
|
||||
+1
-1
@@ -1250,7 +1250,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"path": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"type": {
|
||||
"enum": [
|
||||
|
||||
@@ -1250,7 +1250,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"path": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"type": {
|
||||
"enum": [
|
||||
|
||||
@@ -1250,7 +1250,7 @@
|
||||
"type": "string"
|
||||
},
|
||||
"path": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"type": {
|
||||
"enum": [
|
||||
|
||||
@@ -105,4 +105,4 @@ reasoningEffort: ReasoningEffort | null,
|
||||
/**
|
||||
* Last known status of the target agents, when available.
|
||||
*/
|
||||
agentsStates: { [key in string]?: CollabAgentState }, } | { "type": "subAgentActivity", id: string, kind: SubAgentActivityKind, agentThreadId: string, agentPath: string, } | { "type": "webSearch", id: string, query: string, action: WebSearchAction | null, } | { "type": "imageView", id: string, path: AbsolutePathBuf, } | { "type": "sleep", id: string, durationMs: number, } | { "type": "imageGeneration", id: string, status: string, revisedPrompt: string | null, result: string, savedPath?: AbsolutePathBuf, } | { "type": "enteredReviewMode", id: string, review: string, } | { "type": "exitedReviewMode", id: string, review: string, } | { "type": "contextCompaction", id: string, };
|
||||
agentsStates: { [key in string]?: CollabAgentState }, } | { "type": "subAgentActivity", id: string, kind: SubAgentActivityKind, agentThreadId: string, agentPath: string, } | { "type": "webSearch", id: string, query: string, action: WebSearchAction | null, } | { "type": "imageView", id: string, path: LegacyAppPathString, } | { "type": "sleep", id: string, durationMs: number, } | { "type": "imageGeneration", id: string, status: string, revisedPrompt: string | null, result: string, savedPath?: AbsolutePathBuf, } | { "type": "enteredReviewMode", id: string, review: string, } | { "type": "exitedReviewMode", id: string, review: string, } | { "type": "contextCompaction", id: string, };
|
||||
|
||||
@@ -836,7 +836,7 @@ impl ThreadHistoryBuilder {
|
||||
fn handle_view_image_tool_call(&mut self, payload: &ViewImageToolCallEvent) {
|
||||
let item = ThreadItem::ImageView {
|
||||
id: payload.call_id.clone(),
|
||||
path: payload.path.clone(),
|
||||
path: payload.path.clone().into(),
|
||||
};
|
||||
self.upsert_item_in_current_turn(item);
|
||||
}
|
||||
|
||||
@@ -356,7 +356,10 @@ pub enum ThreadItem {
|
||||
},
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(rename_all = "camelCase")]
|
||||
ImageView { id: String, path: AbsolutePathBuf },
|
||||
ImageView {
|
||||
id: String,
|
||||
path: LegacyAppPathString,
|
||||
},
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(rename_all = "camelCase")]
|
||||
Sleep {
|
||||
@@ -855,7 +858,7 @@ impl From<CoreTurnItem> for ThreadItem {
|
||||
},
|
||||
CoreTurnItem::ImageView(image) => ThreadItem::ImageView {
|
||||
id: image.id,
|
||||
path: image.path,
|
||||
path: image.path.into(),
|
||||
},
|
||||
CoreTurnItem::Sleep(sleep) => ThreadItem::Sleep {
|
||||
id: sleep.id,
|
||||
|
||||
@@ -2574,14 +2574,14 @@ fn core_turn_item_into_thread_item_converts_supported_variants() {
|
||||
|
||||
let image_view_item = TurnItem::ImageView(ImageViewItem {
|
||||
id: "view-image-1".to_string(),
|
||||
path: test_path_buf("/tmp/view-image.png").abs(),
|
||||
path: PathUri::from_abs_path(&test_path_buf("/tmp/view-image.png").abs()),
|
||||
});
|
||||
|
||||
assert_eq!(
|
||||
ThreadItem::from(image_view_item),
|
||||
ThreadItem::ImageView {
|
||||
id: "view-image-1".to_string(),
|
||||
path: test_path_buf("/tmp/view-image.png").abs(),
|
||||
path: LegacyAppPathString::from_abs_path(&test_path_buf("/tmp/view-image.png").abs()),
|
||||
}
|
||||
);
|
||||
|
||||
|
||||
@@ -24,7 +24,6 @@ use crate::tools::registry::CoreToolRuntime;
|
||||
use crate::tools::registry::ToolExecutor;
|
||||
use codex_tools::ToolName;
|
||||
use codex_tools::ToolSpec;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
|
||||
pub struct ViewImageHandler {
|
||||
options: ViewImageToolOptions,
|
||||
@@ -140,36 +139,31 @@ impl ViewImageHandler {
|
||||
"view_image is unavailable in this session".to_string(),
|
||||
));
|
||||
};
|
||||
// TODO(anp): Resolve tool paths using the selected environment's native path convention
|
||||
// so view_image can support relative paths in foreign environments.
|
||||
let cwd = turn_environment.cwd().to_abs_path().map_err(|err| {
|
||||
let path_uri = turn_environment.cwd().join(&path).map_err(|err| {
|
||||
FunctionCallError::RespondToModel(format!(
|
||||
"environment cwd `{}` is not native to the Codex host: {err}",
|
||||
turn_environment.cwd()
|
||||
"unable to resolve image path `{path}` against environment cwd `{}`: {err}",
|
||||
turn_environment.cwd(),
|
||||
))
|
||||
})?;
|
||||
let abs_path = cwd.join(path);
|
||||
let model_visible_path = path_uri.inferred_native_path_string();
|
||||
let sandbox = turn.file_system_sandbox_context(
|
||||
/*additional_permissions*/ None,
|
||||
turn_environment.cwd(),
|
||||
);
|
||||
let fs = turn_environment.environment.get_filesystem();
|
||||
let path_uri = PathUri::from_abs_path(&abs_path);
|
||||
|
||||
let metadata = fs
|
||||
.get_metadata(&path_uri, Some(&sandbox))
|
||||
.await
|
||||
.map_err(|error| {
|
||||
FunctionCallError::RespondToModel(format!(
|
||||
"unable to locate image at `{}`: {error}",
|
||||
abs_path.display()
|
||||
"unable to locate image at `{model_visible_path}`: {error}"
|
||||
))
|
||||
})?;
|
||||
|
||||
if !metadata.is_file {
|
||||
return Err(FunctionCallError::RespondToModel(format!(
|
||||
"image path `{}` is not a file",
|
||||
abs_path.display()
|
||||
"image path `{model_visible_path}` is not a file"
|
||||
)));
|
||||
}
|
||||
let file_bytes = fs
|
||||
@@ -177,11 +171,9 @@ impl ViewImageHandler {
|
||||
.await
|
||||
.map_err(|error| {
|
||||
FunctionCallError::RespondToModel(format!(
|
||||
"unable to read image at `{}`: {error}",
|
||||
abs_path.display()
|
||||
"unable to read image at `{model_visible_path}`: {error}"
|
||||
))
|
||||
})?;
|
||||
let event_path = abs_path.clone();
|
||||
|
||||
let can_request_original_detail = can_request_original_image_detail(&turn.model_info);
|
||||
let use_original_detail =
|
||||
@@ -197,7 +189,7 @@ impl ViewImageHandler {
|
||||
|
||||
let item = TurnItem::ImageView(ImageViewItem {
|
||||
id: call_id,
|
||||
path: event_path,
|
||||
path: path_uri,
|
||||
});
|
||||
session.emit_turn_item_started(turn.as_ref(), &item).await;
|
||||
session.emit_turn_item_completed(turn.as_ref(), item).await;
|
||||
|
||||
@@ -29,7 +29,6 @@ use codex_protocol::protocol::Op;
|
||||
use codex_protocol::protocol::TurnEnvironmentSelection;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use core_test_support::PathBufExt;
|
||||
use core_test_support::PathExt;
|
||||
use core_test_support::get_remote_test_env;
|
||||
use core_test_support::responses;
|
||||
@@ -42,7 +41,6 @@ use core_test_support::responses::mount_sse_sequence;
|
||||
use core_test_support::responses::sse;
|
||||
use core_test_support::responses::start_mock_server;
|
||||
use core_test_support::skip_if_no_network;
|
||||
use core_test_support::skip_if_wine_exec;
|
||||
use core_test_support::test_codex::TestCodex;
|
||||
use core_test_support::test_codex::local;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
@@ -59,8 +57,6 @@ use serde_json::json;
|
||||
use std::fs;
|
||||
use std::io::Cursor;
|
||||
use std::path::PathBuf;
|
||||
use std::time::SystemTime;
|
||||
use std::time::UNIX_EPOCH;
|
||||
use tempfile::TempDir;
|
||||
use tokio::time::Duration;
|
||||
use wiremock::BodyPrintLimit;
|
||||
@@ -295,6 +291,7 @@ async fn view_image_tool_attaches_local_image() -> anyhow::Result<()> {
|
||||
|
||||
let rel_path = "assets/example.png";
|
||||
let abs_path = cwd.join(rel_path);
|
||||
let path_uri = PathUri::from_abs_path(&abs_path);
|
||||
let original_width = 2304;
|
||||
let original_height = 864;
|
||||
write_workspace_png(
|
||||
@@ -369,20 +366,20 @@ async fn view_image_tool_attaches_local_image() -> anyhow::Result<()> {
|
||||
match item_started.expect("view image item started event emitted") {
|
||||
codex_protocol::items::TurnItem::ImageView(item) => {
|
||||
assert_eq!(item.id, call_id);
|
||||
assert_eq!(item.path, abs_path);
|
||||
assert_eq!(item.path, path_uri);
|
||||
}
|
||||
other => panic!("expected ImageView item, got {other:?}"),
|
||||
}
|
||||
match item_completed.expect("view image item completed event emitted") {
|
||||
codex_protocol::items::TurnItem::ImageView(item) => {
|
||||
assert_eq!(item.id, call_id);
|
||||
assert_eq!(item.path, abs_path);
|
||||
assert_eq!(item.path, path_uri);
|
||||
}
|
||||
other => panic!("expected ImageView item, got {other:?}"),
|
||||
}
|
||||
let legacy_event = legacy_event.expect("legacy view image event emitted");
|
||||
assert_eq!(legacy_event.call_id, call_id);
|
||||
assert_eq!(legacy_event.path, abs_path);
|
||||
assert_eq!(legacy_event.path, path_uri);
|
||||
|
||||
let req = mock.single_request();
|
||||
let body = req.body_json();
|
||||
@@ -557,8 +554,10 @@ async fn view_image_tool_applies_local_sandbox_read_denies() -> anyhow::Result<(
|
||||
.function_call_output_content_and_success(call_id)
|
||||
.and_then(|(content, _)| content)
|
||||
.context("sandboxed view_image error text present")?;
|
||||
let expected_locate_prefix = format!("unable to locate image at `{}`:", denied_path.display());
|
||||
let expected_read_prefix = format!("unable to read image at `{}`:", denied_path.display());
|
||||
let denied_path_display =
|
||||
PathUri::from_host_native_path(&denied_path)?.inferred_native_path_string();
|
||||
let expected_locate_prefix = format!("unable to locate image at `{denied_path_display}`:");
|
||||
let expected_read_prefix = format!("unable to read image at `{denied_path_display}`:");
|
||||
assert!(
|
||||
output_text.starts_with(&expected_locate_prefix)
|
||||
|| output_text.starts_with(&expected_read_prefix),
|
||||
@@ -570,8 +569,6 @@ async fn view_image_tool_applies_local_sandbox_read_denies() -> anyhow::Result<(
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn view_image_routes_to_selected_remote_environment() -> anyhow::Result<()> {
|
||||
// TODO(anp): Remove after remote-cwd fixtures use target-native paths.
|
||||
skip_if_wine_exec!(Ok(()), "hardcodes a POSIX remote cwd");
|
||||
skip_if_no_network!(Ok(()));
|
||||
let Some(_remote_env) = get_remote_test_env() else {
|
||||
return Ok(());
|
||||
@@ -583,37 +580,26 @@ async fn view_image_routes_to_selected_remote_environment() -> anyhow::Result<()
|
||||
let local_cwd = TempDir::new()?;
|
||||
fs::write(local_cwd.path().join("remote.png"), b"not a remote image")?;
|
||||
let local_selection = local(local_cwd.path().abs());
|
||||
let remote_cwd = PathBuf::from(format!(
|
||||
"/tmp/codex-view-image-routing-{}",
|
||||
SystemTime::now().duration_since(UNIX_EPOCH)?.as_millis()
|
||||
))
|
||||
.abs();
|
||||
let image_path = remote_cwd.join("remote.png");
|
||||
let remote_cwd_uri = PathUri::from_host_native_path(&remote_cwd)?;
|
||||
test.fs()
|
||||
.create_directory(
|
||||
&remote_cwd_uri,
|
||||
CreateDirectoryOptions { recursive: true },
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
.await?;
|
||||
let remote_cwd_uri = PathUri::from_abs_path(test.executor_environment().cwd());
|
||||
let image_path_uri = remote_cwd_uri.join("remote.png")?;
|
||||
let png = png_bytes(/*width*/ 1, /*height*/ 1, [0, 255, 0, 255])?;
|
||||
let image_path_uri = PathUri::from_host_native_path(&image_path)?;
|
||||
test.fs()
|
||||
.write_file(&image_path_uri, png, /*sandbox*/ None)
|
||||
.await?;
|
||||
let absolute_image_path = image_path_uri.inferred_native_path_string();
|
||||
let remote_selection = TurnEnvironmentSelection {
|
||||
environment_id: REMOTE_ENVIRONMENT_ID.to_string(),
|
||||
cwd: PathUri::from_abs_path(&remote_cwd),
|
||||
cwd: remote_cwd_uri,
|
||||
};
|
||||
let call_id = "call-view-image-multi-env";
|
||||
let relative_call_id = "call-view-image-relative-multi-env";
|
||||
let absolute_call_id = "call-view-image-absolute-multi-env";
|
||||
let response_mock = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-1"),
|
||||
ev_function_call(
|
||||
call_id,
|
||||
relative_call_id,
|
||||
"view_image",
|
||||
&json!({
|
||||
"path": "remote.png",
|
||||
@@ -625,9 +611,22 @@ async fn view_image_routes_to_selected_remote_environment() -> anyhow::Result<()
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-2"),
|
||||
ev_assistant_message("msg-1", "done"),
|
||||
ev_function_call(
|
||||
absolute_call_id,
|
||||
"view_image",
|
||||
&json!({
|
||||
"path": absolute_image_path,
|
||||
"environment_id": REMOTE_ENVIRONMENT_ID,
|
||||
})
|
||||
.to_string(),
|
||||
),
|
||||
ev_completed("resp-2"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-3"),
|
||||
ev_assistant_message("msg-1", "done"),
|
||||
ev_completed("resp-3"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
@@ -638,30 +637,32 @@ async fn view_image_routes_to_selected_remote_environment() -> anyhow::Result<()
|
||||
)
|
||||
.await?;
|
||||
|
||||
let output = response_mock
|
||||
let request = response_mock
|
||||
.last_request()
|
||||
.context("missing request containing view_image output")?
|
||||
.function_call_output(call_id)
|
||||
.clone();
|
||||
let output_items = output
|
||||
.get("output")
|
||||
.and_then(Value::as_array)
|
||||
.context("view_image output should be content items")?;
|
||||
assert_eq!(output_items.len(), 1);
|
||||
let image_url = output_items[0]
|
||||
.get("image_url")
|
||||
.and_then(Value::as_str)
|
||||
.context("view_image output should include image_url")?;
|
||||
assert!(
|
||||
image_url.starts_with("data:image/png;base64,"),
|
||||
"unexpected image_url: {image_url}",
|
||||
);
|
||||
for call_id in [relative_call_id, absolute_call_id] {
|
||||
let output = request.function_call_output(call_id);
|
||||
let output_items = output
|
||||
.get("output")
|
||||
.and_then(Value::as_array)
|
||||
.context("view_image output should be content items")?;
|
||||
assert_eq!(output_items.len(), 1);
|
||||
let image_url = output_items[0]
|
||||
.get("image_url")
|
||||
.and_then(Value::as_str)
|
||||
.context("view_image output should include image_url")?;
|
||||
assert!(
|
||||
image_url.starts_with("data:image/png;base64,"),
|
||||
"unexpected image_url: {image_url}",
|
||||
);
|
||||
}
|
||||
|
||||
test.fs()
|
||||
.remove(
|
||||
&remote_cwd_uri,
|
||||
&image_path_uri,
|
||||
RemoveOptions {
|
||||
recursive: true,
|
||||
recursive: false,
|
||||
force: true,
|
||||
},
|
||||
/*sandbox*/ None,
|
||||
@@ -1174,7 +1175,8 @@ async fn view_image_tool_errors_when_path_is_directory() -> anyhow::Result<()> {
|
||||
.function_call_output_content_and_success(call_id)
|
||||
.and_then(|(content, _)| content)
|
||||
.expect("output text present");
|
||||
let expected_message = format!("image path `{}` is not a file", abs_path.display());
|
||||
let expected_path = PathUri::from_host_native_path(&abs_path)?.inferred_native_path_string();
|
||||
let expected_message = format!("image path `{expected_path}` is not a file");
|
||||
assert_eq!(output_text, expected_message);
|
||||
|
||||
assert!(
|
||||
@@ -1253,19 +1255,26 @@ async fn view_image_tool_turns_invalid_image_into_placeholder() -> anyhow::Resul
|
||||
async fn view_image_tool_errors_when_file_missing() -> anyhow::Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let remote_test_env = get_remote_test_env();
|
||||
println!("view_image missing-file test exec-server environment: {remote_test_env:?}");
|
||||
|
||||
let server = start_mock_server().await;
|
||||
|
||||
let mut builder = test_codex();
|
||||
let test = builder.build_with_remote_env(&server).await?;
|
||||
let TestCodex {
|
||||
codex,
|
||||
config,
|
||||
session_configured,
|
||||
..
|
||||
} = &test;
|
||||
|
||||
let rel_path = "missing/example.png";
|
||||
let abs_path = config.cwd.join(rel_path);
|
||||
// Under wine-exec, the executor cwd is stored as a host-compatible `/C:/...`
|
||||
// projection. Reconstruct its `PathUri` so the expected error uses the selected
|
||||
// environment's native Windows spelling, matching the handler.
|
||||
let expected_path = PathUri::from_abs_path(test.executor_environment().cwd())
|
||||
.join(rel_path)?
|
||||
.inferred_native_path_string();
|
||||
|
||||
let call_id = "view-image-missing";
|
||||
let arguments = serde_json::json!({ "path": rel_path }).to_string();
|
||||
@@ -1309,7 +1318,7 @@ async fn view_image_tool_errors_when_file_missing() -> anyhow::Result<()> {
|
||||
.function_call_output_content_and_success(call_id)
|
||||
.and_then(|(content, _)| content)
|
||||
.expect("output text present");
|
||||
let expected_prefix = format!("unable to locate image at `{}`:", abs_path.display());
|
||||
let expected_prefix = format!("unable to locate image at `{expected_path}`:");
|
||||
assert!(
|
||||
output_text.starts_with(&expected_prefix),
|
||||
"expected error to start with `{expected_prefix}` but got `{output_text}`"
|
||||
|
||||
@@ -25,6 +25,7 @@ use crate::user_input::ByteRange;
|
||||
use crate::user_input::TextElement;
|
||||
use crate::user_input::UserInput;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use quick_xml::de::from_str as from_xml_str;
|
||||
use quick_xml::se::to_string as to_xml_string;
|
||||
use schemars::JsonSchema;
|
||||
@@ -138,7 +139,11 @@ pub struct WebSearchItem {
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, TS, JsonSchema, PartialEq)]
|
||||
pub struct ImageViewItem {
|
||||
pub id: String,
|
||||
pub path: AbsolutePathBuf,
|
||||
/// Path resolved within the selected execution environment.
|
||||
///
|
||||
/// This core protocol type is not exposed directly in the app-server API.
|
||||
/// App-server converts the path to `LegacyAppPathString` at its boundary.
|
||||
pub path: PathUri,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, TS, JsonSchema, PartialEq, Eq)]
|
||||
|
||||
@@ -3399,8 +3399,11 @@ pub struct ExecCommandEndEvent {
|
||||
pub struct ViewImageToolCallEvent {
|
||||
/// Identifier for the originating tool call.
|
||||
pub call_id: String,
|
||||
/// Local filesystem path provided to the tool.
|
||||
pub path: AbsolutePathBuf,
|
||||
/// Filesystem path resolved for the selected environment.
|
||||
///
|
||||
/// This core event is not exposed directly in the app-server API. App-server
|
||||
/// converts the path to `LegacyAppPathString` when building its public item.
|
||||
pub path: PathUri,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, JsonSchema, TS)]
|
||||
|
||||
@@ -182,7 +182,8 @@ fn activity_summary(item: &ThreadItem) -> Option<String> {
|
||||
return bounded_summary(&format!("Web search: {query}"));
|
||||
}
|
||||
ThreadItem::ImageView { path, .. } => {
|
||||
return bounded_summary(&format!("Viewed {}", path.display()));
|
||||
let path = path.render_for_ui();
|
||||
return bounded_summary(&format!("Viewed {path}"));
|
||||
}
|
||||
ThreadItem::ImageGeneration { .. } => return Some("Generated an image".to_string()),
|
||||
ThreadItem::EnteredReviewMode { .. } => return Some("Entered review mode".to_string()),
|
||||
|
||||
+7
@@ -0,0 +1,7 @@
|
||||
---
|
||||
source: tui/src/chatwidget/tests/exec_flow.rs
|
||||
assertion_line: 877
|
||||
expression: combined
|
||||
---
|
||||
• Viewed Image
|
||||
└ C:\workspace\assets\example.png
|
||||
@@ -862,6 +862,21 @@ async fn view_image_tool_call_adds_history_cell() {
|
||||
assert_chatwidget_snapshot!("local_image_attachment_history_snapshot", combined);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn view_image_tool_call_preserves_foreign_path() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
let image_path: LegacyAppPathString =
|
||||
serde_json::from_value(json!(r"C:\workspace\assets\example.png"))
|
||||
.expect("valid legacy app path string");
|
||||
|
||||
handle_view_image_tool_call(&mut chat, "call-image", image_path);
|
||||
|
||||
let cells = drain_insert_history(&mut rx);
|
||||
assert_eq!(cells.len(), 1, "expected a single history cell");
|
||||
let combined = lines_to_single_string(&cells[0]);
|
||||
assert_chatwidget_snapshot!("foreign_image_attachment_history_snapshot", combined);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn image_generation_call_adds_history_cell() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
|
||||
@@ -679,7 +679,7 @@ pub(super) fn handle_patch_apply_end(
|
||||
pub(super) fn handle_view_image_tool_call(
|
||||
chat: &mut ChatWidget,
|
||||
call_id: impl Into<String>,
|
||||
path: AbsolutePathBuf,
|
||||
path: impl Into<LegacyAppPathString>,
|
||||
) {
|
||||
chat.handle_server_notification(
|
||||
ServerNotification::ItemCompleted(ItemCompletedNotification {
|
||||
@@ -688,7 +688,7 @@ pub(super) fn handle_view_image_tool_call(
|
||||
completed_at_ms: 0,
|
||||
item: AppServerThreadItem::ImageView {
|
||||
id: call_id.into(),
|
||||
path,
|
||||
path: path.into(),
|
||||
},
|
||||
}),
|
||||
/*replay_kind*/ None,
|
||||
|
||||
@@ -4,6 +4,7 @@
|
||||
//! events as transcript cells.
|
||||
|
||||
use super::*;
|
||||
use codex_utils_path_uri::LegacyAppPathString;
|
||||
|
||||
impl ChatWidget {
|
||||
pub(super) fn on_patch_apply_begin(&mut self, changes: HashMap<PathBuf, FileChange>) {
|
||||
@@ -11,7 +12,7 @@ impl ChatWidget {
|
||||
self.add_to_history(history_cell::new_patch_event(changes, &self.config.cwd));
|
||||
}
|
||||
|
||||
pub(super) fn on_view_image_tool_call(&mut self, path: AbsolutePathBuf) {
|
||||
pub(super) fn on_view_image_tool_call(&mut self, path: LegacyAppPathString) {
|
||||
self.record_visible_turn_activity();
|
||||
self.flush_answer_stream_with_separator();
|
||||
self.add_to_history(history_cell::new_view_image_tool_call(
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
//! Patch summaries and image-tool transcript helpers.
|
||||
|
||||
use super::*;
|
||||
use codex_utils_path_uri::LegacyAppPathString;
|
||||
|
||||
#[derive(Debug)]
|
||||
pub(crate) struct PatchHistoryCell {
|
||||
@@ -60,8 +61,12 @@ pub(crate) fn new_patch_apply_failure(stderr: String) -> PlainHistoryCell {
|
||||
PlainHistoryCell { lines }
|
||||
}
|
||||
|
||||
pub(crate) fn new_view_image_tool_call(path: AbsolutePathBuf, cwd: &Path) -> PlainHistoryCell {
|
||||
let display_path = display_path_for(path.as_path(), cwd);
|
||||
pub(crate) fn new_view_image_tool_call(path: LegacyAppPathString, cwd: &Path) -> PlainHistoryCell {
|
||||
let display_path = path
|
||||
.to_inferred_path_uri()
|
||||
.and_then(|path| path.to_abs_path().ok())
|
||||
.map(|path| display_path_for(path.as_path(), cwd))
|
||||
.unwrap_or_else(|| path.into_string());
|
||||
|
||||
let lines: Vec<Line<'static>> = vec![
|
||||
vec!["• ".dim(), "Viewed Image".bold()].into(),
|
||||
|
||||
@@ -203,7 +203,8 @@ fn fallback_transcript_cell(item: &ThreadItem) -> Option<PlainHistoryCell> {
|
||||
vec![vec!["web search: ".dim(), query.clone().into()].into()]
|
||||
}
|
||||
ThreadItem::ImageView { path, .. } => {
|
||||
vec![format!("image: {}", path.as_path().display()).dim().into()]
|
||||
let path = path.render_for_ui();
|
||||
vec![format!("image: {path}").dim().into()]
|
||||
}
|
||||
ThreadItem::ImageGeneration {
|
||||
status, saved_path, ..
|
||||
|
||||
@@ -86,6 +86,17 @@ impl LegacyAppPathString {
|
||||
PathUri::try_from(self.clone()).ok()
|
||||
}
|
||||
|
||||
/// Renders this API path for display in a user interface.
|
||||
///
|
||||
/// Absolute paths are normalized using their inferred native convention.
|
||||
/// Strings that cannot be interpreted as absolute paths retain their raw
|
||||
/// API spelling.
|
||||
pub fn render_for_ui(&self) -> String {
|
||||
self.to_inferred_path_uri()
|
||||
.map(|path| path.inferred_native_path_string())
|
||||
.unwrap_or_else(|| self.0.clone())
|
||||
}
|
||||
|
||||
/// Parses this API string as a host-native absolute path.
|
||||
pub fn to_inferred_abs_path(&self) -> Option<AbsolutePathBuf> {
|
||||
AbsolutePathBuf::try_from(self.clone()).ok()
|
||||
|
||||
@@ -458,6 +458,7 @@ fn converts_absolute_api_paths_using_the_inferred_convention() {
|
||||
path.to_inferred_path_uri(),
|
||||
Some(PathUri::parse(expected_uri).expect("expected URI should parse")),
|
||||
);
|
||||
assert_eq!(path.render_for_ui(), raw_path);
|
||||
assert_eq!(
|
||||
PathUri::try_from(path.clone()),
|
||||
path.to_path_uri(convention)
|
||||
|
||||
Reference in New Issue
Block a user