[REWORK] Image Tool#44
Conversation
…utes - Implement `list_image_models` and `list_image_model_providers` admin endpoints - Refactor `ImageGenExecutor` to use central `omniference` engine instead of manual API implementations - Migrate `imagegen` tool settings to use `image_model_id` via a model-picker schema - Add `sqlx::FromRow` derivation to `ModelListAdminRow` for database mapping - Update dependencies in `Cargo.lock` and add database migration for tool settings conversion
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change adds structured assistant message parts, provider-backed image generation, image-model administration, configurable tool system prompts, transactional model synchronization, and frontend support for interleaved reasoning, tool calls, results, and generated images. ChangesStructured chat and image generation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ChatStore
participant StreamingEndpoint
participant OmniEngine
participant ImageGenTool
participant Database
User->>ChatStore: send chat request
ChatStore->>StreamingEndpoint: submit message and enabled tools
StreamingEndpoint->>OmniEngine: send hydrated structured messages
OmniEngine-->>StreamingEndpoint: stream text, reasoning, and tool events
StreamingEndpoint->>ImageGenTool: execute image operation
ImageGenTool->>Database: load model and store generated image
Database-->>ImageGenTool: image_id
ImageGenTool-->>StreamingEndpoint: structured image tool result
StreamingEndpoint->>Database: persist assistant content_parts
StreamingEndpoint-->>ChatStore: stream ordered assistant parts
ChatStore-->>User: render reasoning, text, tools, and images
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| WHEN settings ? 'image_model_id' THEN jsonb_build_object('image_model_id', settings->'image_model_id') | ||
| ELSE '{}'::jsonb |
There was a problem hiding this comment.
When an existing imagegen settings row only has the old api_key/provider/model fields, this migration rewrites it to {}. The new executor requires image_model_id, and an empty per-user row still overrides the admin/system row, so those users keep getting a missing-setting failure even after an admin configures a global image model.
Context Used: AGENTS.md (source)
| let allowed = Model::can_user_use_model(db, &user_id, &model.id) | ||
| .await | ||
| .map_err(|error| ToolError::Internal(format!("Failed to check image-model access: {error}")))?; | ||
| if !allowed { | ||
| return Err(ToolError::ExecutionFailed("You do not have access to the configured image model".to_string())); | ||
| } |
There was a problem hiding this comment.
The image model is selected through an admin-owned tool setting, but execution now checks whether the calling user can use that model directly. A user who can run the imagegen tool can still be rejected if their team lacks access to the globally selected model, so admin-configured image generation fails at runtime for those users even though the tool settings are valid.
Context Used: AGENTS.md (source)
…or tools, and enhanced tool interactions.
…ation/editing functionalities - Add `caption` column to `images` table and `system_prompt` column to `tools` table - Introduce system prompt for the built-in image tool to prevent fabrication of image URLs - Update `imagegen` tool functions (`edit` and `generate`) with enhanced input schema constraints - Add i18n translations for tool editor's system prompt settings (English and German)
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
frontend/components/settings/DefaultModelPicker.vue (1)
175-214: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreload the selected model for
valueMode: 'model_id'.selectedModelonly gets hydrated frompickerModels, and that list is fetched inpickerInit()when the popover opens. With an initialmodelValue, the trigger shows the placeholder/Bot icon on first paint until the dropdown is opened.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/settings/DefaultModelPicker.vue` around lines 175 - 214, Update the modelValue watcher in the DefaultModelPicker setup to hydrate selectedModel for valueMode "model_id" before the popover opens, using the selected-model endpoint and current modelValue when pickerModels does not contain the model. Preserve the existing UUID loading behavior, stale-request guard, null clearing, and pickerModels lookup.src/jobs.rs (1)
124-130: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRun provider sync once before the first sleep.
src/jobs.rs:124-129currently waitsPROVIDER_SYNC_INTERVAL_SECSbefore the firstsync_provider_models_once, so a fresh start/restart leaves models stale until that delay passes. Move the sleep after the sync to preserve the immediate refresh.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jobs.rs` around lines 124 - 130, Update provider_sync_job so sync_provider_models_once runs immediately when the job starts, before entering the interval sleep. Keep the loop repeating by moving the Duration::from_secs(PROVIDER_SYNC_INTERVAL_SECS) sleep after each sync.frontend/components/chat/ToolExecutionDisplay.vue (1)
131-139: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winValidate
image_idformat before splicing it into the image URL.The new branch skips the
isValidImageProtocol/isImageUrlsafety checks used by the other three branches in this same computed property, only checkingtypeof === 'string'. Since this component renders output from any tool (not just the trusted built-inimagegen), an unvalidated string could inject unexpected path segments into the<img src>URL.🛡️ Proposed fix
- if (out?.image_id && typeof out.image_id === 'string') return `/api/v1/images/${out.image_id}`; + if (out?.image_id && typeof out.image_id === 'string' && /^[0-9a-fA-F-]{36}$/.test(out.image_id)) { + return `/api/v1/images/${encodeURIComponent(out.image_id)}`; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/chat/ToolExecutionDisplay.vue` around lines 131 - 139, Update the imageUrl computed property to validate image_id before interpolating it into the API URL, using the existing image URL/protocol validation helpers or an equivalent identifier-specific check. Preserve the current handling of image_url, image_reference, and url, and return null for invalid image_id values.src/types/tools/responses.rs (1)
30-48: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winExclude
system_promptfrom the public tool response.ToolResponse::from_tool_with_functions(...)is used bysrc/routes/public/tools.rs, so authenticated end users receive admin-authored instructions throughsystem_prompt.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/tools/responses.rs` around lines 30 - 48, Remove the system_prompt field from the public ToolResponse struct and update ToolResponse::from_tool_with_functions(...) and any related construction or serialization logic to stop exposing it. Preserve the remaining tool response fields and ensure the public tools route no longer returns admin-authored instructions.src/routes/public/streaming.rs (1)
76-95: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the model-level
max_tokensfallback (src/routes/public/streaming.rs:76-95, 702-703)merge_sampling_with_priorityonly receivesuser_model_config, so requests without a user config row (or withoutmax_output_tokens) now sendmax_tokens = Noneinstead of the model’s declared limit. Pass the system/model config here or keep the old fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/public/streaming.rs` around lines 76 - 95, The max_tokens fallback in merge_sampling_with_priority no longer reaches the declared model limit when user_model_config is absent or lacks max_output_tokens. Update the call site around the streaming request flow to pass the system/model configuration, and adjust merge_sampling_with_priority to use it for the final max_tokens fallback while preserving request and user-config priority.
🧹 Nitpick comments (7)
src/types/models/repository.rs (1)
15-73: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExact-case
"IMAGE"containment check may miss models flagged elsewhere via case-insensitive comparison.Both new queries filter with
COALESCE(m.output_modalities, '[]'::jsonb) @> '["IMAGE"]'::jsonb(lines 24 and 57), which is case-sensitive. Elsewhere in the codebase (e.g.imagegen.rs::load_model,admin/tools.rs::set_tool_settings) the same check is done viamodality.eq_ignore_ascii_case("IMAGE"). If any storedoutput_modalitiesvalues use a different casing, these admin listings will silently exclude image-capable providers/models that pass the Rust-side checks used elsewhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/models/repository.rs` around lines 15 - 73, Update list_image_providers_for_admin and list_image_models_for_admin so their output_modalities filters match IMAGE case-insensitively, consistent with the existing Rust-side checks. Replace the exact-case JSON containment predicates with a SQL expression that normalizes modality values before comparison, while preserving the current null handling and listing behavior.src/main.rs (1)
20-23: 📐 Maintainability & Code Quality | 🔵 TrivialTracing filter won't govern most existing app logs.
The new
EnvFilter/subscriber setup only affectstracing-instrumented output; the codebase's pervasiveprintln!/eprintln!calls (including in the routes and jobs touched by this PR) bypass it entirely and will keep printing unconditionally. If the intent is broader log-level control beyond theomniferencecrate, consider migrating call sites totracing::{info,warn,error}!over time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.rs` around lines 20 - 23, Replace the affected println!/eprintln! logging call sites in the routes and jobs with the appropriate tracing info!, warn!, or error! macros so they are governed by the EnvFilter initialized in main. Preserve each message’s existing content and severity, and use the tracing subscriber rather than adding separate filtering logic.src/utils/tools/builtin/imagegen.rs (1)
109-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMinor: silently swallowed caption write failure.
let _ = crate::utils::images::set_image_caption(...).await;drops the error, unlikerecord_usagein the same file which logs failures viaeprintln!. A failed caption write means later non-vision hydration loses the prompt text silently. Consider logging on error for consistency/observability.♻️ Suggested fix
if !caption.is_empty() { - let _ = crate::utils::images::set_image_caption(db, stored.id, caption).await; + if let Err(error) = crate::utils::images::set_image_caption(db, stored.id, caption).await { + eprintln!("[IMAGEGEN] Failed to set image caption: {error}"); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/tools/builtin/imagegen.rs` around lines 109 - 152, Update the caption persistence branch in store_response to handle errors from set_image_caption instead of discarding them. Log the failure with eprintln!, including the error details and enough context to identify the affected stored image, while preserving successful response handling.frontend/components/chat/MessageItem.vue (1)
330-360: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRecomputes markdown for every interleaved part on each streaming delta.
orderedPartsis a single computed over the wholecontent_partsarray; because it's derived from a reactive array that mutates in place during streaming (e.g.lastPart.text += deltain the store), every incoming delta re-runs.map()over all parts and re-renders markdown (renderStreaming/renderComplete) for parts that already finished, not just the part actually being appended to. For messages with several interleaved reasoning/tool-call/text segments this multiplies rendering work per token.Consider caching rendered HTML per part (e.g., only recompute the last part each tick, keyed by index + text length) instead of remapping the full array every time.
♻️ Sketch of a caching approach
+const partHtmlCache = new Map<number, {text: string; html: string}>(); + const orderedParts = computed(() => { const parts = props.message.content_parts; if (!Array.isArray(parts)) return []; return parts.filter(part => part.type !== 'tool_result').map((part, idx) => { ... if (part.type === 'reasoning' || part.type === 'text') { const text = part.text || ''; - const html = isStreaming.value ? renderStreaming(text) : renderComplete(text); + const cached = partHtmlCache.get(idx); + const html = cached && cached.text === text ? cached.html : (isStreaming.value ? renderStreaming(text) : renderComplete(text)); + partHtmlCache.set(idx, {text, html}); return {type: part.type, html}; } ... }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/chat/MessageItem.vue` around lines 330 - 360, Optimize orderedParts so streaming updates do not rerender markdown for every existing content part. Add per-part caching keyed by a stable identifier or index plus text length/content, reuse cached HTML for unchanged reasoning and text parts, and only invoke renderStreaming or renderComplete for newly changed parts while preserving tool-call, image, filtering, ordering, and streaming behavior.src/utils/images.rs (1)
166-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMinor: add
# Errorsdoc sections.Both new functions return
Resultand can fail on DB errors, but the doc comments omit an# Errorssection. As per coding guidelines, "Include# Errorsand# Panicssections in documentation where applicable."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/images.rs` around lines 166 - 187, Update the documentation comments for set_image_caption and image_caption to add # Errors sections describing that each function returns an error when its database operation fails. Leave the existing behavior and implementation unchanged.Source: Coding guidelines
src/routes/public/streaming.rs (1)
393-411: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winComposite
"tool_name:call_id"naming is duplicated and its downstream parsing is unverified.The
"{name}:{call_id}"format for a tool-role message'snamefield is built in two separate places (tool_messagehere, and inline in the tool-result continuation branch). Per OpenAI's tool-message contract,nameis expected to be the plain function name whiletool_call_idis a separate field;OmniMessagedoesn't appear to expose a distincttool_call_idfield, soomniferencemust be splitting this composite string internally to recover the call id for provider requests. Since this convention already existed pre-PR (previously just the barecall_id) it's likely deliberate and handled correctly downstream, but worth confirming, and duplicating the format string in two places risks drift if the convention ever changes.♻️ Suggested consolidation
+fn tool_message_name(tool_name: &str, call_id: &str) -> String { + format!("{tool_name}:{call_id}") +} + fn tool_message(execs: Option<&Vec<ToolExecutionResponse>>, call_names: &HashMap<String, String>, call_id: &str) -> OmniMessage { ... let name = call_names.get(call_id).map(String::as_str).unwrap_or("tool"); OmniMessage { role: Role::Tool, parts: vec![ContentPart::Text(content)], - name: Some(format!("{name}:{call_id}")), + name: Some(tool_message_name(name, call_id)), } }and reuse
tool_message_name(&exec.tool_name, &exec.call_id)at the other call site.Also applies to: 1163-1182
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/public/streaming.rs` around lines 393 - 411, Confirm the downstream handling of the composite tool-message name format and centralize its construction in a shared helper such as tool_message_name. Update tool_message and the tool-result continuation branch to reuse that helper instead of formatting "{name}:{call_id}" independently, preserving the existing provider request behavior.frontend/types/chat.ts (1)
138-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a discriminated union for
content_parts.The widened type still uses a flat shape with
type: stringand every other field optional, so components can construct/accept invalid combinations (e.g.type: 'text'withtool_call_idset) without a compile error. Matching the backend'sMessagePartenum with a discriminated union would letMessageItem.vue/chatStore.tsget compile-time exhaustiveness instead of relying on runtimeas anycasts.♻️ Suggested type
- content_parts?: Array<{type: string; text?: string; image_id?: string; id?: string; name?: string; arguments?: string; tool_call_id?: string}> | null; + content_parts?: Array< + | {type: 'text'; text: string} + | {type: 'reasoning'; text: string} + | {type: 'image'; image_id: string} + | {type: 'tool_call'; id: string; name: string; arguments?: string} + | {type: 'tool_result'; tool_call_id: string} + > | null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/types/chat.ts` at line 138, Replace the flat content_parts element type with a discriminated union keyed by the backend MessagePart type enum, defining only the fields valid for each part variant and making required fields explicit. Update the related MessageItem.vue and chatStore.ts usages to narrow on the discriminator, remove runtime as any casts where possible, and preserve the existing nullable/optional content_parts contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Around line 85-87: Update the [patch.crates-io] configuration for omniference
to remove the committed ../omniference path override, or document and provision
that required sibling checkout for standalone, CI, and Docker builds. Ensure
omniference resolves through a universally available dependency source by
default.
In `@frontend/components/AppSidebar.vue`:
- Around line 116-117: Move the global settingsReturnPath state from the
standalone useState in AppSidebar.vue into useMainStore(), then update
AppSidebar.vue and settings.vue to read and update the store property while
preserving the existing default path and navigation behavior.
In `@frontend/stores/chatStore.ts`:
- Around line 746-757: Update the tool_name condition in the content_parts image
handling block to also accept imagegen_edit, while preserving the existing
imagegen and imagegen_generate cases and image_id validation.
In `@migrations/20260711000000_provider_backed_image_tool.sql`:
- Around line 16-24: The migration’s UPDATE currently discards existing imagegen
settings when image_model_id is absent. Update the imagegen backfill query to
preserve the existing settings object and add the configured default
image_model_id, retaining any other settings; use the project’s established
default model value rather than replacing settings with an empty JSON object.
In `@src/routes/admin/tools.rs`:
- Around line 410-419: Replace the raw SQL query in the affected admin tool
handler with the existing Tool::find_by_id_system repository method. Preserve
the NotFound response when no tool is returned, but propagate or explicitly
handle repository/database errors as InternalError with logging consistent with
update_tool and create_tool; do not use .ok().flatten() or access state.db
directly from the route.
- Around line 420-438: Remove the imagegen-specific model/provider validation
and persistence logic from the route handler around the builtin imagegen branch,
including its direct database lookups and raw INSERT. Delegate settings
validation and persistence to the existing tools/types-layer function or method,
keeping the handler responsible only for request handling and invoking that
abstraction; preserve the existing user-access guard during execution.
In `@src/routes/public/streaming.rs`:
- Around line 359-391: The hydrate_image path causes repeated sequential
database reads and base64 hydration for every historical image on each vision
request. Update get_omni_messages and hydrate_image to avoid rehydrating the
full image history on every request, using an appropriate cap, cache, or batched
lookup while preserving current generated-image handles and visibility behavior.
In `@src/types/models/repository.rs`:
- Around line 32-69: Update the SQL in list_image_models_for_admin so each ILIKE
condition uses the same single-backslash ESCAPE literal as the other queries in
this file. Keep the existing search binding and escaping behavior unchanged.
In `@src/utils/tools/builtin/imagegen.rs`:
- Around line 93-106: Restrict the remote-fetch branch of the image input loader
before `self.client.get(value)`: parse and validate the URL, reject loopback,
private, link-local, and multicast destinations, and re-check the resolved
address to prevent DNS rebinding. Enforce a maximum response size before reading
the body instead of unbounded `response.bytes()`, and use a consistent generic
fetch error for blocked, oversized, and failed requests to avoid exposing
distinct probing signals. Keep the local `/api/v1/images/` database path
unchanged.
- Around line 70-77: The load_model access check must not reject users for the
admin-managed image_model_id default. Replace the direct
Model::can_user_use_model validation in the builtin imagegen path with the
access rule for tool-owned/admin-selected settings, while preserving error
handling for genuine model-loading failures and allowing enabled users to use
the configured default.
---
Outside diff comments:
In `@frontend/components/chat/ToolExecutionDisplay.vue`:
- Around line 131-139: Update the imageUrl computed property to validate
image_id before interpolating it into the API URL, using the existing image
URL/protocol validation helpers or an equivalent identifier-specific check.
Preserve the current handling of image_url, image_reference, and url, and return
null for invalid image_id values.
In `@frontend/components/settings/DefaultModelPicker.vue`:
- Around line 175-214: Update the modelValue watcher in the DefaultModelPicker
setup to hydrate selectedModel for valueMode "model_id" before the popover
opens, using the selected-model endpoint and current modelValue when
pickerModels does not contain the model. Preserve the existing UUID loading
behavior, stale-request guard, null clearing, and pickerModels lookup.
In `@src/jobs.rs`:
- Around line 124-130: Update provider_sync_job so sync_provider_models_once
runs immediately when the job starts, before entering the interval sleep. Keep
the loop repeating by moving the
Duration::from_secs(PROVIDER_SYNC_INTERVAL_SECS) sleep after each sync.
In `@src/routes/public/streaming.rs`:
- Around line 76-95: The max_tokens fallback in merge_sampling_with_priority no
longer reaches the declared model limit when user_model_config is absent or
lacks max_output_tokens. Update the call site around the streaming request flow
to pass the system/model configuration, and adjust merge_sampling_with_priority
to use it for the final max_tokens fallback while preserving request and
user-config priority.
In `@src/types/tools/responses.rs`:
- Around line 30-48: Remove the system_prompt field from the public ToolResponse
struct and update ToolResponse::from_tool_with_functions(...) and any related
construction or serialization logic to stop exposing it. Preserve the remaining
tool response fields and ensure the public tools route no longer returns
admin-authored instructions.
---
Nitpick comments:
In `@frontend/components/chat/MessageItem.vue`:
- Around line 330-360: Optimize orderedParts so streaming updates do not
rerender markdown for every existing content part. Add per-part caching keyed by
a stable identifier or index plus text length/content, reuse cached HTML for
unchanged reasoning and text parts, and only invoke renderStreaming or
renderComplete for newly changed parts while preserving tool-call, image,
filtering, ordering, and streaming behavior.
In `@frontend/types/chat.ts`:
- Line 138: Replace the flat content_parts element type with a discriminated
union keyed by the backend MessagePart type enum, defining only the fields valid
for each part variant and making required fields explicit. Update the related
MessageItem.vue and chatStore.ts usages to narrow on the discriminator, remove
runtime as any casts where possible, and preserve the existing nullable/optional
content_parts contract.
In `@src/main.rs`:
- Around line 20-23: Replace the affected println!/eprintln! logging call sites
in the routes and jobs with the appropriate tracing info!, warn!, or error!
macros so they are governed by the EnvFilter initialized in main. Preserve each
message’s existing content and severity, and use the tracing subscriber rather
than adding separate filtering logic.
In `@src/routes/public/streaming.rs`:
- Around line 393-411: Confirm the downstream handling of the composite
tool-message name format and centralize its construction in a shared helper such
as tool_message_name. Update tool_message and the tool-result continuation
branch to reuse that helper instead of formatting "{name}:{call_id}"
independently, preserving the existing provider request behavior.
In `@src/types/models/repository.rs`:
- Around line 15-73: Update list_image_providers_for_admin and
list_image_models_for_admin so their output_modalities filters match IMAGE
case-insensitively, consistent with the existing Rust-side checks. Replace the
exact-case JSON containment predicates with a SQL expression that normalizes
modality values before comparison, while preserving the current null handling
and listing behavior.
In `@src/utils/images.rs`:
- Around line 166-187: Update the documentation comments for set_image_caption
and image_caption to add # Errors sections describing that each function returns
an error when its database operation fails. Leave the existing behavior and
implementation unchanged.
In `@src/utils/tools/builtin/imagegen.rs`:
- Around line 109-152: Update the caption persistence branch in store_response
to handle errors from set_image_caption instead of discarding them. Log the
failure with eprintln!, including the error details and enough context to
identify the affected stored image, while preserving successful response
handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 79f624d5-b38d-4aff-9c5a-5fdb35ac625a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
Cargo.tomlfrontend/components/AppSidebar.vuefrontend/components/chat/MessageItem.vuefrontend/components/chat/ToolExecutionDisplay.vuefrontend/components/settings/DefaultModelPicker.vuefrontend/composables/useModelPicker.tsfrontend/pages/settings.vuefrontend/pages/settings/tools.vuefrontend/stores/chatStore.tsfrontend/types/chat.tsmigrations/20260711000000_provider_backed_image_tool.sqlmigrations/20260712000000_image_tool_i18n.sqlmigrations/20260713000000_image_tool_editing_and_tool_system_prompt.sqlsrc/jobs.rssrc/main.rssrc/routes/admin/models.rssrc/routes/admin/tools.rssrc/routes/mod.rssrc/routes/public/streaming.rssrc/tests/models.rssrc/types/chat/mod.rssrc/types/chat/repository.rssrc/types/chat/requests.rssrc/types/models/repository.rssrc/types/models/rows.rssrc/types/tools/mod.rssrc/types/tools/repository.rssrc/types/tools/requests.rssrc/types/tools/responses.rssrc/utils/images.rssrc/utils/tools/builtin/imagegen.rs
…ge generation security, and tool settings validation - Update sampling logic to include model-level defaults - Add security checks for external image URLs in `imagegen` tool - Optimize tool settings validation and persistence - Refactor content part handling in assistant messages - Upgrade omniference dependency to latest commit
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/components/chat/MessageItem.vue (1)
346-364: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winExhaust the
imagevariant before accessingpart.text.
image_idis typed asstring, sopart.type === 'image' && part.image_idcan leave an image variant with''in the fallthrough. Type checking then rejectspart.text; at runtime it would render an undefined text value. Match solely onpart.type === 'image'.Proposed fix
- if (part.type === 'image' && part.image_id) { + if (part.type === 'image') {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/chat/MessageItem.vue` around lines 346 - 364, Update the image branch in the part-mapping logic to match solely on part.type === 'image', ensuring every image variant returns through the image result before the shared text-rendering fallthrough accesses part.text.
🧹 Nitpick comments (2)
src/types/tools/mod.rs (1)
14-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public error type.
ToolSettingsErroris public but lacks a///doc comment describing its use.As per coding guidelines,
src/**/*.rs: “Document public APIs with///doc comments.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/tools/mod.rs` around lines 14 - 22, Add a concise /// documentation comment to the public ToolSettingsError enum describing that it represents failures when loading or validating tool settings. Leave its existing variants and error messages unchanged.Source: Coding guidelines
src/types/tools/repository.rs (1)
11-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
set_system_settingsand its failure contract.Add
///docs with an# Errorssection covering missing tools, invalid image-model settings, and database failures.As per coding guidelines,
src/**/*.rs: “Document public APIs with///doc comments” and “Include# Errorsand# Panicssections in documentation where applicable.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/tools/repository.rs` around lines 11 - 15, Add a `///` documentation comment to the public `set_system_settings` function, describing its purpose and including an `# Errors` section that documents failures for missing tools, invalid image-model settings, and database errors. Do not alter the function’s behavior or signature.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@migrations/20260711000000_provider_backed_image_tool.sql`:
- Around line 19-35: Update the backfill query joining models and providers to
restrict candidates to enabled providers and models that support image output,
reusing the schema’s existing capability and enabled-state fields. Ensure each
tool-settings record selects exactly one deterministic candidate, using an
explicit stable ordering or equivalent per-record selection, while preserving
the existing provider/model matching rules and avoiding invalid global
overrides.
---
Outside diff comments:
In `@frontend/components/chat/MessageItem.vue`:
- Around line 346-364: Update the image branch in the part-mapping logic to
match solely on part.type === 'image', ensuring every image variant returns
through the image result before the shared text-rendering fallthrough accesses
part.text.
---
Nitpick comments:
In `@src/types/tools/mod.rs`:
- Around line 14-22: Add a concise /// documentation comment to the public
ToolSettingsError enum describing that it represents failures when loading or
validating tool settings. Leave its existing variants and error messages
unchanged.
In `@src/types/tools/repository.rs`:
- Around line 11-15: Add a `///` documentation comment to the public
`set_system_settings` function, describing its purpose and including an `#
Errors` section that documents failures for missing tools, invalid image-model
settings, and database errors. Do not alter the function’s behavior or
signature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3bedbbea-325b-41c4-9a09-54ef0dcfdbf2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
Cargo.tomlfrontend/components/AppSidebar.vuefrontend/components/chat/MessageItem.vuefrontend/components/chat/ToolExecutionDisplay.vuefrontend/components/settings/DefaultModelPicker.vuefrontend/pages/settings.vuefrontend/stores/chatStore.tsfrontend/stores/index.tsfrontend/types/chat.tsmigrations/20260711000000_provider_backed_image_tool.sqlsrc/jobs.rssrc/routes/admin/tools.rssrc/routes/public/streaming.rssrc/routes/public/tools.rssrc/types/models/repository.rssrc/types/tools/mod.rssrc/types/tools/repository.rssrc/types/tools/responses.rssrc/utils/images.rssrc/utils/tools/builtin/imagegen.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- frontend/components/AppSidebar.vue
- frontend/components/chat/ToolExecutionDisplay.vue
- src/types/tools/responses.rs
- frontend/pages/settings.vue
- src/jobs.rs
- src/utils/images.rs
- src/types/models/repository.rs
- frontend/components/settings/DefaultModelPicker.vue
- frontend/stores/chatStore.ts
- src/routes/public/streaming.rs
- src/utils/tools/builtin/imagegen.rs
| JOIN models m ON true | ||
| JOIN providers p ON p.id = m.provider_id | ||
| WHERE uts.tool_id = t.id | ||
| AND t.source_kind = 'BUILTIN' | ||
| AND t.source_config->>'builtin_id' = 'imagegen' | ||
| AND NOT uts.settings ? 'image_model_id' | ||
| AND m.model_id = COALESCE( | ||
| uts.settings->>'model', | ||
| CASE LOWER(COALESCE(uts.settings->>'provider', 'openai')) | ||
| WHEN 'google' THEN 'imagen-3.0-generate-002' | ||
| ELSE 'dall-e-3' | ||
| END | ||
| ) | ||
| AND ( | ||
| (LOWER(COALESCE(uts.settings->>'provider', 'openai')) = 'openai' AND p.kind = 'OPENAI') | ||
| OR (LOWER(uts.settings->>'provider') = 'google' AND p.kind = 'GOOGLE') | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Backfill only a deterministic, selectable image model.
This can persist an ID for a disabled provider/model or a model without image output; multiple matching provider rows can also make PostgreSQL choose an arbitrary candidate. That bypasses Tool::set_system_settings validation and leaves invalid global overrides. Restrict candidates to enabled image-capable models/providers and select one deterministic row per settings record.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@migrations/20260711000000_provider_backed_image_tool.sql` around lines 19 -
35, Update the backfill query joining models and providers to restrict
candidates to enabled providers and models that support image output, reusing
the schema’s existing capability and enabled-state fields. Ensure each
tool-settings record selects exactly one deterministic candidate, using an
explicit stable ordering or equivalent per-record selection, while preserving
the existing provider/model matching rules and avoiding invalid global
overrides.
| @@ -523,7 +551,7 @@ const builtinToolTemplates = [ | |||
| { | |||
There was a problem hiding this comment.
This block has no translations
| input_schema = COALESCE($7, input_schema), | ||
| settings_schema = COALESCE($8, settings_schema), | ||
| is_enabled = COALESCE($9, is_enabled), | ||
| system_prompt = COALESCE($10, system_prompt), |
There was a problem hiding this comment.
No database requests in route files (see AGENTS.md)
There was a problem hiding this comment.
No database requests in route files (see AGENTS.md)
…ogic, improve localization, and unify usage of the `tracing` library for logging.
1 participant
Refactor imagegen tool to use provider-backed models and add admin routes
list_image_modelsandlist_image_model_providersadmin endpointsImageGenExecutorto use centralomniferenceengine instead of manual API implementationsimagegentool settings to useimage_model_idvia a model-picker schemasqlx::FromRowderivation toModelListAdminRowfor database mappingCargo.lockand add database migration for tool settings conversionGreptile Summary
This PR moves image generation onto provider-backed model records. The main changes are:
image_model_id.Confidence Score: 4/5
The imagegen settings migration and runtime access path need fixes before merging.
migrations/20260711000000_provider_backed_image_tool.sql, src/utils/tools/builtin/imagegen.rs
Important Files Changed
image_model_id, but legacy rows can become empty overrides.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Admin participant UI as Settings UI participant API as Admin API participant DB participant User participant Exec as ImageGenExecutor participant OF as Omniference Admin->>UI: Select image model UI->>API: Save image_model_id API->>DB: Validate and store tool settings User->>Exec: Run imagegen Exec->>DB: Resolve effective settings Exec->>DB: Load model and provider Exec->>DB: Check caller model access Exec->>OF: Send image request OF-->>Exec: Return image bytes Exec->>DB: Store image Exec-->>User: Return image URL%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Admin participant UI as Settings UI participant API as Admin API participant DB participant User participant Exec as ImageGenExecutor participant OF as Omniference Admin->>UI: Select image model UI->>API: Save image_model_id API->>DB: Validate and store tool settings User->>Exec: Run imagegen Exec->>DB: Resolve effective settings Exec->>DB: Load model and provider Exec->>DB: Check caller model access Exec->>OF: Send image request OF-->>Exec: Return image bytes Exec->>DB: Store image Exec-->>User: Return image URLReviews (1): Last reviewed commit: "Refactor imagegen tool to use provider-b..." | Re-trigger Greptile
Context used (4)
Summary by CodeRabbit
New Features
Improvements