Route image edits through referenced file paths#26486
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0165dbf6ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn image_url(path: &AbsolutePathBuf) -> Result<ImageUrl, FunctionCallError> { | ||
| let bytes = std::fs::read(path).map_err(|error| { |
There was a problem hiding this comment.
Validate image references before reading model-supplied paths
When a prompt-injected or mistaken model call includes any absolute path here, the tool now reads that path and forwards the decoded image to the image edit backend without checking that the path was one of the local images or generated artifacts exposed in the current conversation. In contexts where the process can read other valid image files on disk, this lets the model exfiltrate those images simply by setting referenced_image_paths; the description-only instruction is not an enforcement boundary. Please allowlist the requested paths against the conversation’s known image paths before calling std::fs::read.
Useful? React with 👍 / 👎.
| codex-protocol = { workspace = true } | ||
| codex-tools = { workspace = true } | ||
| codex-utils-absolute-path = { workspace = true } | ||
| codex-utils-image = { workspace = true } |
There was a problem hiding this comment.
Refresh Bazel lock after adding crate dependency
This adds a new Rust dependency edge for codex-image-generation-extension, but the diff only updates Cargo.lock and does not include MODULE.bazel.lock, so Bazel-based builds/checks can see stale cargo metadata. The repo guidance requires running just bazel-lock-update and including the refreshed lockfile whenever Cargo.toml or Cargo.lock changes (AGENTS.md lines 34-35).
Useful? React with 👍 / 👎.
| enum ImagegenAction { | ||
| Generate, | ||
| Edit, | ||
| referenced_image_paths: Option<Vec<AbsolutePathBuf>>, |
There was a problem hiding this comment.
Preserve edits for remote image inputs
In app-server/TUI flows where the user input is a UserInput::Image remote/data URL, the model still sees an image but there is no local path it can place in referenced_image_paths; this new argument shape has no way to carry the existing image URL, so those edit requests either turn into generations when the list is omitted/empty or force the user to re-upload. The old selector used ContentItem::InputImage from history, so remote-image edits that were already in the conversation regress.
Useful? React with 👍 / 👎.
| enum ImagegenAction { | ||
| Generate, | ||
| Edit, | ||
| referenced_image_paths: Option<Vec<AbsolutePathBuf>>, |
There was a problem hiding this comment.
Accept relative referenced image paths
When a user attaches an image with a relative path, for example through the CLI path that forwards --image ./foo.png as a raw PathBuf, the prompt can show that same relative path but this field deserializes as AbsolutePathBuf without an AbsolutePathBufGuard, which rejects non-absolute paths before the edit can run. The previous edit path used the already-materialized InputImage from history, so relative local-image edits now fail unless the user happened to provide an absolute path.
Useful? React with 👍 / 👎.
| enum ImagegenAction { | ||
| Generate, | ||
| Edit, | ||
| referenced_image_paths: Option<Vec<AbsolutePathBuf>>, |
There was a problem hiding this comment.
Keep legacy imagegen action arguments resumable
Existing rollouts that were interrupted after the model emitted an imagegen call but before the tool output was recorded can still contain the old {"action":"generate"|"edit","prompt":...} arguments; after this schema change, deny_unknown_fields makes those resumed calls fail as soon as action is parsed. The repo’s breaking-change review surface includes “resuming sessions from existing rollouts” (AGENTS.md lines 92-99), so please keep a compatibility path for legacy action calls.
Useful? React with 👍 / 👎.
sayan-oai
left a comment
There was a problem hiding this comment.
lgtm, get jimmy's review.
| path.display() | ||
| )) | ||
| })?; | ||
| let image = load_for_prompt_bytes(path.as_path(), bytes, PromptImageMode::Original).map_err( |
There was a problem hiding this comment.
This still isn’t using the exact image snapshot the model saw. Local images are already converted into ContentItem::InputImage before the model request, but the edit path rereads the file later
So if the file is deleted/replaced/... before the tc, we edit could different bytes
There was a problem hiding this comment.
hm ok we can parse through convo history and attach the image snapshot that's in the ContentItem::InputImage i think!
| "`num_last_images_to_include` must be between 1 and {MAX_EDIT_IMAGES}" | ||
| ))); | ||
| } | ||
| let images = recent_images(history, count); |
There was a problem hiding this comment.
num_last_images_to_include still can’t identify the exact pathless image the model selected. If the target is the second-most-recent image, count = 2 also sends the unrelated newest image to /image(s)/edit. Can we use stable image refs or indices so only the selected images are sent?
There was a problem hiding this comment.
left a comment in code
|
good to go for now |
Why
Image edits should use the exact images selected by the model instead of inferring edit inputs from conversation history.
What changed
actionargument with optionalreferenced_image_paths.Validation
just codexwith copy pasted image, attached image, etc.just test -p codex-image-generation-extensionjust test -p codex-app-server standalone_image_edit_uses_attached_model_visible_imagejust fix -p codex-image-generation-extensionjust bazel-lock-check