-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Expose local image paths to models #25944
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4bd82b8
9ba487e
71bc830
9100c4a
64a8cb2
cd92f42
6d77965
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1019,9 +1019,10 @@ pub fn local_image_label_text(label_number: usize) -> String { | |
| format!("[Image #{label_number}]") | ||
| } | ||
|
|
||
| pub fn local_image_open_tag_text(label_number: usize) -> String { | ||
| pub fn local_image_open_tag_text_with_path(label_number: usize, path: &std::path::Path) -> String { | ||
| let label = local_image_label_text(label_number); | ||
| format!("{LOCAL_IMAGE_OPEN_TAG_PREFIX}{label}{LOCAL_IMAGE_OPEN_TAG_SUFFIX}") | ||
| let path = path.display(); | ||
| format!("{LOCAL_IMAGE_OPEN_TAG_PREFIX}{label} path=\"{path}\"{LOCAL_IMAGE_OPEN_TAG_SUFFIX}") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the local image filename contains characters such as Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the local image filename will not ocntain these characters..
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This sends the full Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| pub fn is_local_image_open_tag_text(text: &str) -> bool { | ||
|
|
@@ -1080,7 +1081,7 @@ pub fn local_image_content_items_with_label_number( | |
| let mut items = Vec::with_capacity(3); | ||
| if let Some(label_number) = label_number { | ||
| items.push(ContentItem::InputText { | ||
| text: local_image_open_tag_text(label_number), | ||
| text: local_image_open_tag_text_with_path(label_number, path), | ||
| }); | ||
| } | ||
| items.push(ContentItem::InputImage { | ||
|
|
@@ -2866,7 +2867,7 @@ mod tests { | |
| detail: None, | ||
| }, | ||
| UserInput::LocalImage { | ||
| path: local_path, | ||
| path: local_path.clone(), | ||
| detail: None, | ||
| }, | ||
| ]); | ||
|
|
@@ -2883,7 +2884,10 @@ mod tests { | |
| assert_eq!( | ||
| content.get(1), | ||
| Some(&ContentItem::InputText { | ||
| text: local_image_open_tag_text(/*label_number*/ 2), | ||
| text: local_image_open_tag_text_with_path( | ||
| /*label_number*/ 2, | ||
| &local_path | ||
| ), | ||
| }) | ||
| ); | ||
| assert!(matches!( | ||
|
|
@@ -2903,6 +2907,17 @@ mod tests { | |
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn local_image_open_tag_preserves_path() { | ||
| assert_eq!( | ||
| local_image_open_tag_text_with_path( | ||
| /*label_number*/ 1, | ||
| std::path::Path::new(r#"/tmp/a&"<b>.png"#), | ||
| ), | ||
| r#"<image name=[Image #1] path="/tmp/a&"<b>.png">"# | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn local_image_user_input_preserves_requested_detail() -> Result<()> { | ||
| let dir = tempdir()?; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For local images under very long absolute paths, this writes the entire path into a new model-visible
InputTextitem, and the string has no hard cap before it becomes part of the request context. The model-context review rule requires every injected item to be bounded and flags new individual items that can exceed 1k tokens as P0; see AGENTS.md lines 87-90. Please truncate or otherwise cap only the displayed path while keeping the full path in non-model state.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extremely unlikely for the path (codex home + thread_id + call_id) to get to any level that is concernin