Repository navigation
feat(core): add image_generate tool - #59939
Conversation
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks sound. I couldn't run it end to end because there are no image-provider credentials. With the PR, the new tests pass (8), as do the nearby tool-opencode, tool-registry and test/plugin suites (482), and tsc -b in packages/core is clean.
Public API: adds a built-in image_generate tool with a matching image_generate permission action (it asks by default), and adds an optional type input (chat | image | decision, default chat) to the models tool. The output shape doesn't change. All of this is backwards compatible, and no config, SDK or HTTP API changes.
One thing worth fixing, inline: when the provider returns JPEG or WebP for a .png path, the file is written to a path the edit permission never approved, and the existence check for that path only runs after the paid generation.
| const bytes = Buffer.from(image.data, "base64") | ||
| const mime = Mime.detect(bytes) | ||
| // Providers may return JPEG or WebP; core has no encoder, so the file keeps its real type. | ||
| const extension = EXTENSIONS[mime] ?? path.extname(requested) |
There was a problem hiding this comment.
When path is given (say logo.png) and the provider returns JPEG or WebP, the file is written to logo.jpg. But the edit permission (and authorizeExternal) was asked for logo.png, so a rule that would deny or ask for the actual target is never checked. Also, if logo.jpg already exists, the error at line 265 comes only after the paid generation, and the image is thrown away.
Could an explicit path keep the exact name the user asked for, with the note saying the content is JPEG? Another option is to re-check permission and existence for the renamed path before writing. Changing the extension only for the default temp path would keep the current behaviour there.
There was a problem hiding this comment.
The earlier point is addressed: an explicit path is now saved exactly as given, so the file written is the one the edit permission approved and the overwrite check covered. Only the default temp path takes the real extension, and its name is new each time. I checked this with the PR's tests (8 pass, including the new explicit-path case) and tsc -b in packages/core is clean; I couldn't run it live because there are no image-provider credentials.
Public API impact is unchanged from the last review: a new built-in image_generate tool and permission action, and an optional type input (chat | image | decision, default chat) for the models tool. All of it is backwards compatible. Nothing new to raise.
There was a problem hiding this comment.
The new commit is fine. It removes aspect and resolution cleanly, and the descriptions no longer say the result is always a PNG, which matches how non-PNG results are saved now. The earlier thread about the renamed path stays fixed: an explicit path is saved exactly as given. I checked it with the PR's tests (8 pass) and tsc -b in packages/core is clean; I couldn't run it live because there are no image-provider credentials.
Public API: a new built-in image_generate tool (prompt, images, model, path) and permission action, plus an optional type input (chat | image | decision, default chat) for the models tool. All of it is backwards compatible. Nothing new to raise.
There was a problem hiding this comment.
The new commit is fine. Dropping the dimension parsing makes the tool smaller, nothing else depended on dimensions, and the tests were updated to match. The earlier thread about the renamed path stays fixed: an explicit path is saved exactly as given. I checked it with the PR's tests (8 pass) and tsc -b in packages/core is clean; I couldn't run it live because there are no image-provider credentials.
Public API: a new built-in image_generate tool (input prompt, images, model, path; output path, model) and permission action, plus an optional type input (chat | image | decision, default chat) for the models tool. All of it is backwards compatible. Nothing new to raise.
bc4dc9e to
cdc3b35
Compare
There was a problem hiding this comment.
The new experimental.image_generation setting looks right. The configured default gives way to an explicit model, falls back with a note when it can't serve the call, and turning the tool off in config takes effect on config.updated without a restart. The earlier thread about the renamed path stays fixed. I checked it with the image-generate and config normalization tests (52 pass) and tsc -b in packages/core is clean; I couldn't run it live because there are no image-provider credentials. Running bun run generate in packages/client changed nothing, so the committed OpenAPI and client types are up to date.
Public API: a new built-in image_generate tool (input prompt, images, model, path; output path, model) and permission action, an optional type input for the models tool, and a new config key experimental.image_generation (false | { model?: ModelSelection }), which is reflected in the OpenAPI spec and the client ConfigEntry type. All of it is backwards compatible. Nothing new to raise.
cdc3b35 to
3c5678f
Compare
There was a problem hiding this comment.
Running the generation as a background job looks sound. Everything that needs the user or can fail fast (choosing the model, the overwrite check, both permission prompts, reading the input images) still happens before the job starts, so a denial costs nothing. The job follows the background-shell pattern, including restart recovery and keeping the location loaded. The earlier thread about the renamed path stays fixed. I checked it with the image-generate, session-execution, config, job, location-activity and shell tests (no failures) and tsc -b in packages/core is clean; I couldn't run it live because there are no image-provider credentials.
Public API is unchanged since the last review: the image_generate tool and permission action, an optional type input for the models tool, and the experimental.image_generation config key (false | { model? }). The new image recovery kind is only stored internally by the job service. All of it is backwards compatible. Nothing new to raise.
3c5678f to
e2d1036
Compare
There was a problem hiding this comment.
Variant support mostly matches the subagent tool: an unknown explicit variant fails and lists the ones that exist, a configured variant the model doesn't have falls back with a note, and the variant reaches Generate.image. The earlier thread about the renamed path stays fixed. I checked it with the image-generate and session-execution tests (50 pass) and tsc -b in packages/core is clean; I couldn't run it live because there are no image-provider credentials.
Public API: the image_generate model input and experimental.image_generation.model now also accept providerID/modelID#variant. Everything else is unchanged from the last review (the image_generate tool and permission action, an optional type input for the models tool, the experimental.image_generation config key). All of it is backwards compatible.
One concern, inline: because the variant is part of the permission resource, adding #variant gets around a permission rule written for that exact model.
There was a problem hiding this comment.
This commit only rewrites the models tool descriptions (what each type is for, and that the tool finds references for any tool that takes a model). Nothing changes in behaviour, and the related tests pass. Public API unchanged. The point about the variant in the permission resource is still open.
There was a problem hiding this comment.
This commit is fine. The firmer no-polling wording only changes text, and the subagent tool now says when a requested model is an image or decision model instead of "not available", checking only after the chat lookup fails. The tests pass with the two new cases, and tsc -b in packages/core is clean. Public API unchanged. The point about the variant in the permission resource is still open.
27a84f3 to
268e340
Compare
There was a problem hiding this comment.
This commit is fine. Input image URLs now go through the webfetch permission before any download and before the paid job, and the new test checks the permission order. The default model is now simply the newest from the session's provider, otherwise the newest overall. Image models are already sorted newest first, so that holds. Dropping decision from the models tool's type is safe since it hasn't shipped. The tests pass and tsc -b in packages/core is clean.
Public API: the same as before except that the models tool's type input is now chat | image. All of it is backwards compatible. The point about the variant in the permission resource is still open.
268e340 to
0e1861a
Compare
There was a problem hiding this comment.
The rename to experimental.image_model is fine, and the generated client and OpenAPI files are up to date. The image-generate and config normalization tests pass (56).
Blocking: after the rebase, tsc -b in packages/core fails. src/session/background.ts, which arrived from dev with the goal feature, assumes every background job is either a shell or a subagent, so the new image kind fails to compile there (5 errors). At runtime an image job would be treated as a subagent with no parentSessionID, so it never counts as background work for the experimental.idle hook or the goal reviewer. outstanding() needs an image case using recovery.sessionID, and either an image entry in SessionIdleBackground or a deliberate skip.
Public API: the new config key is now experimental.image_model (a model selection) instead of experimental.image_generation. It hasn't shipped, so nothing breaks. The rest is unchanged: the image_generate tool and permission action, and an optional type (chat | image) for the models tool. The point about the variant in the permission resource is still open.
There was a problem hiding this comment.
The type errors from the rebase are fixed. session/background.ts handles image jobs, SessionIdleBackground has an image entry, and the goal plugin waits on the Session's own image job the same way it does for its subagent, since the image notice wakes it again. tsc -b in packages/core is clean, and the image-generate, goal, session-runner and session-execution tests pass.
Public API: in addition to the earlier list (the image_generate tool and permission action, an optional type for the models tool, and experimental.image_model), the experimental SessionIdleBackground plugin type gains a kind: "image" entry. That's an addition, so it's backwards compatible; only a plugin that assumes it has handled every kind may need to handle it. The point about the variant in the permission resource is still open.
613c17e to
8e611c9
Compare
There was a problem hiding this comment.
This push is a rebase onto newer dev with no changes to the PR's own code. tsc -b in packages/core stays clean, and the image-generate, session-execution and goal tests pass. Public API unchanged from the last review: the image_generate tool and permission action, an optional type for the models tool, experimental.image_model, and the image kind in SessionIdleBackground, all backwards compatible. Nothing new to raise.
There was a problem hiding this comment.
Saving with the real extension and checking the real file against deny rules addresses my earlier point. Overwriting is now explicit, so a paid image is no longer thrown away because a file already exists. The image-generate and session-execution tests pass (50), and tsc -b in packages/core is clean. Public API unchanged apart from the image_generate behaviour itself: its output path is now the path without an extension.
One gap, inline: when the provider's format differs from the requested extension, an existing file with the real extension is overwritten without the user ever being asked.
| if (!image) return yield* Effect.fail(new Error(`${ref} returned no image.`)) | ||
| const bytes = Buffer.from(image.data, "base64") | ||
| const file = stem + (EXTENSIONS[Mime.detect(bytes)] ?? ".png") | ||
| // The edit prompt covered the requested path; a different extension must still not be denied. |
There was a problem hiding this comment.
assertNotDenied only blocks an explicit deny, so this file is otherwise written without anyone being asked. If the user approves logo.png and the provider returns JPEG, an existing logo.jpg is overwritten even though the edit prompt only covered logo.png. Regenerating an image next to an older one with another extension is a pretty normal case.
Could this ask for the edit permission (permission.assert) when the real file differs and already exists? Another option is to write it under a new name, such as logo-1.jpg, instead of replacing the existing file.
I added a built-in
image_generatetool so the model can make or edit images with whatever image model the user has, and see the result.Visual
Problem
Core can already call image models through
Generate.image, but the model had no way to use it. Getting an image meant a plugin with a fixed provider and model.Change
New tool in
packages/core/src/tool/plugin/image-generate.ts. Everything that needs the user or can fail fast happens during the call; the paid generation then runs as a background job, so the call returns right away.The job is registered with a durable
imagerecovery entry, the same way background shells are. If the server stops mid-generation, the job is cancelled and the next boot tells the session ("...was cancelled because the server restarted"). A result that finished but wasn't delivered yet gets delivered then. A running image job also keeps its location loaded, like a background shell.Inputs:
prompt(required) andimages(1 to 8 paths or https URLs, in order). Local paths go through the normal read permission and https URLs through thewebfetchpermission, so denying web access also covers image downloads. There's no size or shape input, so each model uses its default size.modelasproviderID/modelIDorproviderID/modelID#variant, only when the user asks for one. It's handled like the subagent tool'smodel: an unknown variant fails with the list of available ones, and the variant is passed toGenerate.imageso core applies its settings. The description points the model at the models tool instead of listing model names, so it stays the same when models change and the prompt cache holds.pathfor the image file, relative to the session directory or absolute. Defaults to<tmp>/images/<sessionID>/image_<id>. The saved file always takes the extension of the image's real format (.png,.jpg,.webp,.gif), whatever the requested path ends in, and an existing file is overwritten after the edit permission check. The completion notice gives the exact path.Default model when
modelis omitted:When an explicit model can't take input images and images were passed, the error names up to 3 image models that can.
Registration: the tool is only registered while at least one image model is available It re-checks on
model.updatedand reloads the tool registry when that flips, so turning a provider on or off adds or drops the tool without a restart, and no model sees a tool it can't use.Config: a new
experimental.image_modelsetting: the default image model, in the same format as the top-levelmodelsetting (variant included). A configured model or variant that isn't usable falls back to automatic picking with a note. It's read at call time, so edits apply without a restart. To turn the tool off, denyimage_generatein permissions like any other tool. Client types and the OpenAPI document are regenerated.{ "experimental": { // default image model when the agent doesn't pick one "image_model": "openai/gpt-image-2" } }Models tool:
modelsgets an optionaltype(chatdefault, orimage), with a short description of each, so the model can find image model IDs. Decision models are left out for now, since no built-in tool can use them. Output shape is unchanged.Subagent: the
modelfield now says it must be a chat model, and passing an image or decision model fails with "X is an image/decision model; subagents need a chat model" instead of "not available".File format: images are saved as the provider returned them, with the matching extension. If that changes the extension of an explicit
path, the edit prompt covered the requested path and the actual file is still checked against deny rules.Out of scope
n > 1), and converting non-PNG output to PNGVerification
bun typecheckinpackages/core: cleanbun run script/test.ts test/tool-image-generate.test.ts --rerun-each 5: 65 pass. Covers the immediate result, the completion notice on success, failure and cancellation, no job when a permission is denied, registration following the catalog and config, the configured default model and its fallback notes, default model order, explicit model validation, input images from files, permission order, overwriting an explicit path, the default temp path, JPEG output on default and explicit paths, and the models tooltypefilter.bun run testinpackages/core: everything passes except 7 shell tests, which fail the same way ondevlocally (local bash version)test/session-execution.test.ts: new case for restart delivery of a finished image result and cancelling a running onebun test test/config/normalization.test.ts: passes, includingfalse,{ model }and invalid valuesbun run checkat the root: passesopencode run --standalonewith an OpenAI key ongpt-5.4-mini(before the background change): the tool pickedopenai/gpt-image-2.5-flareand saved a 1024x1024 PNGAdded by the opencode bot (AI-generated).
Public API: adds the image_generate tool and permission action, an optional
typeinput to the models tool, the experimental.image_model config key, and animagekind in the plugin SessionIdleBackground type (backwards compatible)