mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[3 of 3] Support images in TUI goals (#27510)
## Stack 1. [1 of 3] Support long raw TUI goal objectives - #27508 2. [2 of 3] Support long pasted text in TUI goals - #27509 3. **[3 of 3] Support images in TUI goals** - this PR ## Why The first two PRs make goal definitions resilient to long text, but `/goal` still dropped image inputs from the composer. That meant a user could attach images while defining a goal and the resulting goal continuation would not have any useful reference to those images. Goal state still persists only objective text, so image inputs need to become paths or URLs that the agent can read later. ## What Changed - Extends TUI `GoalDraft` with local image attachments and remote image URLs. - Copies local goal images through the app-server filesystem layer into the managed goal attachment directory, then rewrites active image placeholders to file references. - Appends unplaced local images and remote image URLs to the objective as referenced image files or URLs. - Preserves goal image metadata through live `/goal` submission and queued `/goal` dispatch. ## Verification - Added goal materialization coverage for local image files and remote image URLs. - Added/updated TUI slash-command coverage showing `/goal` drafts include attached images instead of dropping them. ## Manual Testing - Attached an image by bracketed-pasting its local path into a live `/goal` composer. The `[Image #1]` placeholder became a server-host `image-1.png` reference, copied bytes matched exactly, and no attachment was written under the TUI's local home. - Deleted an image placeholder before submitting a small goal and verified no image was copied. - Attached PNG and JPEG files to the same goal. Placeholder order was preserved as `image-1.png` and `image-2.jpg`, and both remote copies matched their source bytes. - Tried extensionless, malformed-extension, and extension/content-mismatched paths; the composer rejected them as image attachments before goal dispatch rather than creating misleading managed image files. - Combined a local image, a large pasted block, and enough raw text to exceed 4,000 characters. The remote attachment directory contained the image, paste sidecar, and `goal-objective.md`; all embedded references used server-host paths and both payloads matched their sources. - Submitted an image replacement while a goal was active, verified no image was copied before confirmation, then canceled and confirmed the attachment count was unchanged.
This commit is contained in:
committed by
GitHub
Unverified
parent
bacfc5e4c0
commit
9e07892253
@@ -4221,6 +4221,7 @@ async fn set_thread_goal_draft_materializes_long_objective_and_confirms_before_p
|
||||
Some(placeholder.to_string()),
|
||||
)],
|
||||
pending_pastes: vec![(placeholder.to_string(), "hello".to_string())],
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
app.set_thread_goal_draft(
|
||||
@@ -4293,6 +4294,7 @@ async fn set_thread_goal_draft_materializes_long_objective_and_confirms_before_p
|
||||
Some(whitespace_placeholder.to_string()),
|
||||
)],
|
||||
pending_pastes: vec![(whitespace_placeholder.to_string(), " \n\t".to_string())],
|
||||
..Default::default()
|
||||
},
|
||||
crate::app_event::ThreadGoalSetMode::ReplaceExisting,
|
||||
)
|
||||
@@ -4310,6 +4312,48 @@ async fn set_thread_goal_draft_materializes_long_objective_and_confirms_before_p
|
||||
.objective,
|
||||
"small goal"
|
||||
);
|
||||
|
||||
let image_dir = tempfile::tempdir()?;
|
||||
let image_path = image_dir.path().join("local-image.png");
|
||||
std::fs::write(&image_path, b"png bytes")?;
|
||||
let image_placeholder = "[Image #3]";
|
||||
app.set_thread_goal_draft(
|
||||
&mut app_server,
|
||||
thread_id,
|
||||
crate::goal_files::GoalDraft {
|
||||
objective: format!("Describe {image_placeholder}"),
|
||||
text_elements: vec![TextElement::new(
|
||||
(9..9 + image_placeholder.len()).into(),
|
||||
Some(image_placeholder.to_string()),
|
||||
)],
|
||||
local_images: vec![crate::bottom_pane::LocalImageAttachment {
|
||||
placeholder: image_placeholder.to_string(),
|
||||
path: image_path,
|
||||
}],
|
||||
remote_image_urls: vec![
|
||||
"https://example.com/first.png".to_string(),
|
||||
"https://example.com/second.png".to_string(),
|
||||
],
|
||||
..Default::default()
|
||||
},
|
||||
crate::app_event::ThreadGoalSetMode::ReplaceExisting,
|
||||
)
|
||||
.await;
|
||||
let objective = app_server
|
||||
.thread_goal_get(thread_id)
|
||||
.await?
|
||||
.goal
|
||||
.expect("image goal should be set")
|
||||
.objective;
|
||||
let copied_image = objective
|
||||
.strip_prefix("Describe image file: ")
|
||||
.and_then(|text| text.split_once("\n\n"))
|
||||
.map(|(path, _)| path)
|
||||
.expect("copied image path");
|
||||
assert_eq!(std::fs::read(copied_image)?, b"png bytes");
|
||||
assert!(objective.contains(
|
||||
"Referenced image URLs:\n- [Image #1]: https://example.com/first.png\n- [Image #2]: https://example.com/second.png"
|
||||
));
|
||||
app_server.shutdown().await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -829,7 +829,6 @@ impl BottomPane {
|
||||
self.composer.text_elements()
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
pub(crate) fn composer_local_images(&self) -> Vec<LocalImageAttachment> {
|
||||
self.composer.local_images()
|
||||
}
|
||||
@@ -879,7 +878,6 @@ impl BottomPane {
|
||||
self.request_redraw();
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
pub(crate) fn remote_image_urls(&self) -> Vec<String> {
|
||||
self.composer.remote_image_urls()
|
||||
}
|
||||
|
||||
@@ -562,8 +562,8 @@ impl ChatWidget {
|
||||
args,
|
||||
text_elements,
|
||||
pending_pastes: self.bottom_pane.composer_pending_pastes(),
|
||||
local_images: Vec::new(),
|
||||
remote_image_urls: Vec::new(),
|
||||
local_images: self.bottom_pane.composer_local_images(),
|
||||
remote_image_urls: self.bottom_pane.remote_image_urls(),
|
||||
mention_bindings: Vec::new(),
|
||||
source: SlashCommandDispatchSource::Live,
|
||||
},
|
||||
@@ -775,6 +775,8 @@ impl ChatWidget {
|
||||
objective: args,
|
||||
text_elements,
|
||||
pending_pastes,
|
||||
local_images,
|
||||
remote_image_urls,
|
||||
};
|
||||
let Some(thread_id) = self.thread_id else {
|
||||
if source == SlashCommandDispatchSource::Live {
|
||||
@@ -792,8 +794,8 @@ impl ChatWidget {
|
||||
self.queue_user_message_with_options(
|
||||
UserMessage {
|
||||
text: format!("{GOAL_PREFIX}{}", draft.objective),
|
||||
local_images: Vec::new(),
|
||||
remote_image_urls: Vec::new(),
|
||||
local_images: draft.local_images,
|
||||
remote_image_urls: draft.remote_image_urls,
|
||||
text_elements,
|
||||
mention_bindings: Vec::new(),
|
||||
},
|
||||
|
||||
@@ -658,30 +658,38 @@ async fn goal_slash_command_uses_plain_text_for_mentions() {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn goal_slash_command_drops_attached_images() {
|
||||
async fn goal_slash_command_emits_attached_images() {
|
||||
let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
chat.set_feature_enabled(Feature::Goals, /*enabled*/ true);
|
||||
let thread_id = ThreadId::new();
|
||||
chat.thread_id = Some(thread_id);
|
||||
let remote_url = "https://example.com/goal.png".to_string();
|
||||
let local_image = PathBuf::from("/tmp/goal-local.png");
|
||||
let local_image = chat.config.codex_home.join("goal-local.png");
|
||||
std::fs::write(&local_image, b"png bytes").expect("write local image");
|
||||
let placeholder = "[Image #2]";
|
||||
let command = format!("/goal describe {placeholder}");
|
||||
let placeholder_start = command.find(placeholder).expect("placeholder in command");
|
||||
chat.set_remote_image_urls(vec![remote_url]);
|
||||
let command = format!("/goal literal {placeholder} describe {placeholder}");
|
||||
let placeholder_start = command.rfind(placeholder).expect("placeholder in command");
|
||||
chat.set_remote_image_urls(vec![remote_url.clone()]);
|
||||
chat.bottom_pane.set_composer_text(
|
||||
command,
|
||||
vec![TextElement::new(
|
||||
(placeholder_start..placeholder_start + placeholder.len()).into(),
|
||||
Some(placeholder.to_string()),
|
||||
)],
|
||||
vec![local_image],
|
||||
vec![local_image.to_path_buf()],
|
||||
);
|
||||
|
||||
chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
|
||||
|
||||
let draft = next_goal_draft(&mut rx, thread_id);
|
||||
assert_eq!(draft.objective, "describe [Image #2]");
|
||||
assert_eq!(
|
||||
draft.objective,
|
||||
format!("literal {placeholder} describe {placeholder}")
|
||||
);
|
||||
assert_eq!(draft.remote_image_urls, vec![remote_url]);
|
||||
assert_eq!(draft.local_images.len(), 1);
|
||||
assert_eq!(draft.local_images[0].placeholder, placeholder);
|
||||
assert_eq!(draft.local_images[0].path, local_image.to_path_buf());
|
||||
assert!(chat.remote_image_urls().is_empty());
|
||||
assert!(chat.bottom_pane.composer_local_image_paths().is_empty());
|
||||
assert_no_submit_op(&mut op_rx);
|
||||
|
||||
@@ -1,7 +1,11 @@
|
||||
//! Materializes oversized TUI goal objectives and pastes as app-server-host files.
|
||||
//! Materializes oversized TUI goal objectives, pastes, and images as app-server-host files.
|
||||
|
||||
use std::fs;
|
||||
use std::path::Path;
|
||||
|
||||
use crate::app_server_session::AppServerSession;
|
||||
use crate::bottom_pane::ChatComposer;
|
||||
use crate::bottom_pane::LocalImageAttachment;
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
use anyhow::bail;
|
||||
@@ -20,6 +24,8 @@ pub(crate) struct GoalDraft {
|
||||
pub(crate) objective: String,
|
||||
pub(crate) text_elements: Vec<TextElement>,
|
||||
pub(crate) pending_pastes: Vec<(String, String)>,
|
||||
pub(crate) local_images: Vec<LocalImageAttachment>,
|
||||
pub(crate) remote_image_urls: Vec<String>,
|
||||
}
|
||||
|
||||
pub(crate) type GoalFilePath = AppServerPath;
|
||||
@@ -71,9 +77,46 @@ pub(crate) async fn materialize_goal_draft(
|
||||
));
|
||||
}
|
||||
|
||||
let mut image_lines = Vec::new();
|
||||
for (idx, image) in draft.local_images.iter().enumerate() {
|
||||
if !image.placeholder.is_empty() {
|
||||
let Some(active_idx) = active_placeholders
|
||||
.iter()
|
||||
.position(|active| *active == image.placeholder.as_str())
|
||||
else {
|
||||
continue;
|
||||
};
|
||||
active_placeholders.swap_remove(active_idx);
|
||||
}
|
||||
let extension = image_extension(&image.path);
|
||||
let path = ensure_goal_output_dir(app_server, codex_home, &mut output_dir)
|
||||
.await?
|
||||
.join(format!("image-{}.{}", idx + 1, extension));
|
||||
let bytes = fs::read(&image.path)
|
||||
.with_context(|| format!("Could not read goal image {}", image.path.display()))?;
|
||||
write_goal_file(app_server, path.clone(), bytes).await?;
|
||||
if image.placeholder.is_empty() {
|
||||
image_lines.push(format!("- [Image #{}]: {path}", idx + 1));
|
||||
} else {
|
||||
replacements.push((image.placeholder.clone(), format!("image file: {path}")));
|
||||
}
|
||||
}
|
||||
|
||||
let (expanded_objective, _) =
|
||||
ChatComposer::expand_pending_pastes(&objective, text_elements, &replacements);
|
||||
objective = expanded_objective.trim().to_string();
|
||||
append_section(&mut objective, "Referenced image files:", image_lines);
|
||||
|
||||
append_section(
|
||||
&mut objective,
|
||||
"Referenced image URLs:",
|
||||
draft
|
||||
.remote_image_urls
|
||||
.into_iter()
|
||||
.enumerate()
|
||||
.map(|(idx, url)| format!("- [Image #{}]: {url}", idx + 1))
|
||||
.collect(),
|
||||
);
|
||||
|
||||
if objective.chars().count() > MAX_THREAD_GOAL_OBJECTIVE_CHARS {
|
||||
let path = ensure_goal_output_dir(app_server, codex_home, &mut output_dir)
|
||||
@@ -172,3 +215,28 @@ async fn write_goal_file(
|
||||
.map_err(|err| anyhow::anyhow!("{err}"))
|
||||
.with_context(|| format!("Could not write goal file {path}"))
|
||||
}
|
||||
fn append_section(objective: &mut String, heading: &str, lines: Vec<String>) {
|
||||
if lines.is_empty() {
|
||||
return;
|
||||
}
|
||||
if !objective.ends_with('\n') {
|
||||
objective.push_str("\n\n");
|
||||
}
|
||||
objective.push_str(heading);
|
||||
objective.push('\n');
|
||||
objective.push_str(&lines.join("\n"));
|
||||
}
|
||||
|
||||
fn image_extension(path: &Path) -> String {
|
||||
path.extension()
|
||||
.and_then(|extension| extension.to_str())
|
||||
.map(|extension| {
|
||||
extension
|
||||
.chars()
|
||||
.filter(char::is_ascii_alphanumeric)
|
||||
.take(8)
|
||||
.collect::<String>()
|
||||
})
|
||||
.filter(|extension| !extension.is_empty())
|
||||
.unwrap_or_else(|| "png".to_string())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user