Route extension image generation through the native image completion pipeline#24972
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ad8939d49
ℹ️ 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".
| "image generation returned no image data".to_string(), | ||
| )); | ||
| }; | ||
| let output_hint = | ||
| match persist_generated_image(&self.output_dir, &call.call_id, &result).await { | ||
| Ok(output_hint) => Some(output_hint), | ||
| Err(err) => { | ||
| tracing::warn!( | ||
| call_id = %call.call_id, | ||
| output_dir = %self.output_dir.display(), | ||
| "failed to save generated image: {err}" | ||
| ); | ||
| None | ||
| } | ||
| }; | ||
| let output_hint = call | ||
| .turn_item_emitter |
There was a problem hiding this comment.
Emit image generation start before awaiting the backend
For long generate/edit requests, this is the first point where the extension publishes a native image-generation item, but it runs only after backend.generate/edit(...).await has already returned. CoreTurnItemEmitter::image_generation_completed then emits ItemStarted and ItemCompleted back-to-back, so users do not get the native in-progress image-generation UI while the image API call is actually running. Consider emitting the started image item before awaiting the backend and only completing it here.
Useful? React with 👍 / 👎.
| }) | ||
| } | ||
|
|
||
| fn image_generation_completed<'a>( |
There was a problem hiding this comment.
we want to reuse the core persistence logic. that is what this does (good), but IMO its a smell that we're defining such imagegen-persistence-specific logic in the generic extension_tool.rs and tool_call.rs.
could we adjust the extension API (as @jif-oai suggested here and i punted 🙃) so an extension can publish an unfinalized item into the existing core TurnItem processing logic? then both hosted ResponseItem and extension TurnItem processing could use the same path.
i imagine the extension could still own the new save-location info in the functioncall output.
There was a problem hiding this comment.
Yes I would like something a bit cleaner here...
We can discuss it further on Slack but basically extension-api should be agnostic of those types
jif-oai
left a comment
There was a problem hiding this comment.
This is very hacky! The follow-up will be very important
| use pretty_assertions::assert_eq; | ||
| use tokio as _; // Keep dev-dep for cargo-shear; tests use #[tokio::test]. |
There was a problem hiding this comment.
Wtf? Just drop the dependency then, why keeping this like that?
There was a problem hiding this comment.
codex thought we still used tokio::test and cargo-shear was hallucinating lol it was the other way around, merging after removing this
| pub type TurnItemEmissionFuture<'a> = Pin<Box<dyn Future<Output = ()> + Send + 'a>>; | ||
|
|
||
| /// Future returned when an image-generation extension publishes completed image bytes. | ||
| pub type ImageGenerationCompletionFuture<'a> = |
There was a problem hiding this comment.
Just for the record, this also needs to drop in the follow-up
Why
The standalone
image_gen.imagegenextension should behave like native image generation for artifact persistence and UI completion, while returning its save-location guidance as part of the tool result instead of injecting a developer message.What Changed
ImageGenerationlifecycle events.FunctionCallOutput, alongside the generated image input for model follow-up.Fast Follow
TurnItemfinalization path for re-usability of codeValidation
just fmt.just bazel-lock-update.just bazel-lock-check.just test -p codex-tools -p codex-extension-api -p codex-image-generation-extension.just test -p codex-core image_generation_publication_is_finalized_by_core.just test -p codex-core handle_output_item_done_records_image_save_history_message.just fix -p codex-tools -p codex-extension-api -p codex-core -p codex-image-generation-extension.