feat(attachments): image attachment support for vision-capable models (#4644) - #4871
Merged
Merged
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (51)
💤 Files with no reviewable changes (25)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds end-to-end multimodal image attachment support: ChangesMultimodal Image Attachment Pipeline
Sequence Diagram(s)sequenceDiagram
participant Browser
participant AxumRouter
participant RebornServices
participant InboundAttachmentReader
participant ScopedFilesystem
Browser->>AxumRouter: GET /api/webchat/v2/threads/{t}/messages/{m}/attachments/{a}
AxumRouter->>RebornServices: read_attachment(caller, request)
RebornServices->>RebornServices: resolve_thread_history_for_caller (automation scope fallback)
RebornServices->>RebornServices: find AttachmentRef by attachment_id in history
RebornServices->>InboundAttachmentReader: read(thread_scope, storage_key)
InboundAttachmentReader->>ScopedFilesystem: read_bytes_bounded(scoped_path)
ScopedFilesystem-->>InboundAttachmentReader: bytes
InboundAttachmentReader-->>RebornServices: Vec~u8~
RebornServices-->>AxumRouter: RebornAttachmentBytes { mime_type, bytes }
AxumRouter-->>Browser: 200 Content-Type, nosniff, Cache-Control, bytes
sequenceDiagram
participant ThreadBackedLoopModelPort
participant LoopAttachmentReadPort
participant ModelGateway
participant AnthropicBedrock
ThreadBackedLoopModelPort->>LoopAttachmentReadPort: read_attachment_bytes(scope, storage_key)
LoopAttachmentReadPort-->>ThreadBackedLoopModelPort: Vec~u8~ bytes
ThreadBackedLoopModelPort->>ThreadBackedLoopModelPort: build HostManagedModelImagePart
ThreadBackedLoopModelPort->>ModelGateway: HostManagedModelMessage { image_parts }
ModelGateway->>ModelGateway: is_vision_model? base64 encode → data: URL
ModelGateway->>AnthropicBedrock: ChatMessage::user_with_parts [text + ImageUrl]
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
|
github-actions
Bot
added
size: M
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for multimodal image attachments on messages by adding a ContextImageAttachment struct and an image_attachments field to ContextMessage. It implements a helper function model_image_attachments to filter and map image attachments, and populates this field across the in-memory and filesystem-based thread services. The reviewer recommends adding unit tests for the new model_image_attachments helper function to ensure correctness and prevent regressions, providing a sample test case.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+29
to
+31
| pub(crate) fn model_image_attachments( | ||
| attachments: &[AttachmentRef], | ||
| ) -> Vec<ContextImageAttachment> { |
Contributor
There was a problem hiding this comment.
The new helper function model_image_attachments is introduced to filter and map image attachments, but it currently lacks dedicated unit test coverage. To ensure correctness and prevent future regressions, please add a unit test in the tests module.
Here is a suggested test case:
#[test]
fn test_model_image_attachments() {
let attachments = vec![
AttachmentRef {
id: "att-1".to_string(),
kind: AttachmentKind::Image,
mime_type: "image/png".to_string(),
filename: Some("test.png".to_string()),
size_bytes: Some(100),
storage_key: Some("path/to/test.png".to_string()),
extracted_text: None,
},
AttachmentRef {
id: "att-2".to_string(),
kind: AttachmentKind::Document,
mime_type: "application/pdf".to_string(),
filename: Some("test.pdf".to_string()),
size_bytes: Some(200),
storage_key: Some("path/to/test.pdf".to_string()),
extracted_text: None,
},
AttachmentRef {
id: "att-3".to_string(),
kind: AttachmentKind::Image,
mime_type: "image/jpeg".to_string(),
filename: Some("test.jpg".to_string()),
size_bytes: Some(150),
storage_key: None,
extracted_text: None,
},
];
let image_attachments = model_image_attachments(&attachments);
assert_eq!(image_attachments.len(), 1);
assert_eq!(image_attachments[0].mime_type, "image/png");
assert_eq!(image_attachments[0].storage_key, "path/to/test.png");
}
github-actions
Bot
added
scope: dependencies
ilblackdragon
marked this pull request as ready for review
June 15, 2026 04:43
ilblackdragon
enabled auto-merge
June 15, 2026 04:44
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an end-to-end “landed image attachment → read-back → base64 encode → multimodal message parts” path for Reborn so vision-capable models receive real image content (via ContentPart::ImageUrl data URLs) while text-only models continue to see the existing <attachments> text pointer.
Changes:
- Extend thread context projection to carry image attachment references (
ContextMessage.image_attachments) derived from landedAttachmentRefs. - Add a loop-side attachment read port (
LoopAttachmentReadPort) and plumb it through Reborn runtime composition to read scoped attachment bytes and base64-encode them into transientimage_parts. - Update the Reborn model gateway to emit
ChatMessage::user_with_parts(...)for vision models and add unit/integration tests for the new image path.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/support/reborn/harness.rs | Updates harness wiring to include the new attachment_read_port field. |
| tests/support_unit_tests.rs | Updates test fixtures to include image_parts. |
| crates/ironclaw_threads/src/lib.rs | Re-exports ContextImageAttachment from the contract. |
| crates/ironclaw_threads/src/in_memory.rs | Populates ContextMessage.image_attachments for in-memory store context reads. |
| crates/ironclaw_threads/src/filesystem_service.rs | Populates ContextMessage.image_attachments for filesystem store context reads. |
| crates/ironclaw_threads/src/contract.rs | Adds ContextImageAttachment and ContextMessage.image_attachments. |
| crates/ironclaw_threads/src/attachment_context.rs | Documents the split between <attachments> text and the multimodal image path; adds helper to extract model-visible image refs. |
| crates/ironclaw_reborn/tests/loop_driver_host.rs | Updates planned runtime construction calls to pass attachment_read_port. |
| crates/ironclaw_reborn/tests/llm_gateway.rs | Updates gateway tests to include image_parts in HostManagedModelMessage. |
| crates/ironclaw_reborn/src/runtime.rs | Threads optional LoopAttachmentReadPort into host factory composition. |
| crates/ironclaw_reborn/src/model_gateway.rs | Converts resolved image parts into ContentPart::ImageUrl for vision models; adds unit tests for vision vs non-vision behavior. |
| crates/ironclaw_reborn/src/loop_driver_host/model_gateway.rs | Plumbs attachment_read_port into ThreadBackedLoopModelPort construction. |
| crates/ironclaw_reborn/src/loop_driver_host.rs | Adds builder support for installing an attachment read port into the host. |
| crates/ironclaw_reborn_composition/tests/product_live_adapters.rs | Updates composition readiness test wiring for attachment_read_port. |
| crates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rs | Updates local dev runtime tests to include image_parts. |
| crates/ironclaw_reborn_composition/src/runtime.rs | Wires ProjectScopedAttachmentReader into build_reborn_runtime when a local workspace filesystem exists. |
| crates/ironclaw_reborn_composition/src/attachment_landing.rs | Adds ProjectScopedAttachmentReader implementing LoopAttachmentReadPort + unit tests for readback/error taxonomy. |
| crates/ironclaw_product_workflow/tests/support/planned_agent_loop.rs | Updates product workflow planned loop harness to pass attachment_read_port. |
| crates/ironclaw_product_workflow/tests/inbound_turn_contract.rs | Updates inbound turn contract tests to pass attachment_read_port. |
| crates/ironclaw_loop_support/tests/thread_loop_support_contract.rs | Adds integration coverage that the model port reads/encodes image bytes into image_parts. |
| crates/ironclaw_loop_support/src/system_inference.rs | Updates constructed HostManagedModelMessage fixtures to include image_parts. |
| crates/ironclaw_loop_support/src/prompt_context_budget.rs | Updates context message test helper to include image_attachments. |
| crates/ironclaw_loop_support/src/lib.rs | Introduces LoopAttachmentReadPort, image part DTOs, and model-port logic to read+base64 encode image attachments into transient image_parts. |
| crates/ironclaw_loop_support/Cargo.toml | Adds base64 dependency for image encoding. |
| Cargo.lock | Locks the new base64 dependency version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+463
to
+467
| /// This is a *reference*, never bytes: `storage_key` is the landed scoped path | ||
| /// (e.g. `/workspace/attachments/...`); the turn layer reads the bytes back | ||
| /// through the project filesystem authority only when the active model can | ||
| /// actually accept images. The textual `<attachments>` pointer in | ||
| /// [`ContextMessage::content`] remains the fallback for text-only models. |
Comment on lines
+1479
to
+1482
| /// An image attachment already read back and base64-encoded, ready to become a | ||
| /// multimodal content part for a vision-capable model. Populated only when the | ||
| /// resolved model accepts images (see the model-message resolution path); the | ||
| /// model gateway turns each into a `ContentPart::ImageUrl` data URL. |
Comment on lines
+1531
to
+1533
| /// Encoded image attachments for the multimodal path. Empty unless the | ||
| /// resolved model is vision-capable. Not serialized (transient turn data). | ||
| #[serde(default, skip)] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/ironclaw_loop_support/src/lib.rs`:
- Around line 1479-1520: The LoopAttachmentReadError enum implements Display but
not std::error::Error, which limits error propagation patterns. Implement the
std::error::Error trait for LoopAttachmentReadError. Since the enum already has
a Display implementation, the Error trait implementation can be minimal
(typically just returning None from the source() method unless you want to
support error chains). This enables better error handling patterns downstream
while maintaining current functionality.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: f7aae2d7-dd30-4e71-a665-f1498f014964
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (24)
crates/ironclaw_loop_support/Cargo.tomlcrates/ironclaw_loop_support/src/lib.rscrates/ironclaw_loop_support/src/prompt_context_budget.rscrates/ironclaw_loop_support/src/system_inference.rscrates/ironclaw_loop_support/tests/thread_loop_support_contract.rscrates/ironclaw_product_workflow/tests/inbound_turn_contract.rscrates/ironclaw_product_workflow/tests/support/planned_agent_loop.rscrates/ironclaw_reborn/src/loop_driver_host.rscrates/ironclaw_reborn/src/loop_driver_host/model_gateway.rscrates/ironclaw_reborn/src/model_gateway.rscrates/ironclaw_reborn/src/runtime.rscrates/ironclaw_reborn/tests/llm_gateway.rscrates/ironclaw_reborn/tests/loop_driver_host.rscrates/ironclaw_reborn_composition/src/attachment_landing.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rscrates/ironclaw_reborn_composition/tests/product_live_adapters.rscrates/ironclaw_threads/src/attachment_context.rscrates/ironclaw_threads/src/contract.rscrates/ironclaw_threads/src/filesystem_service.rscrates/ironclaw_threads/src/in_memory.rscrates/ironclaw_threads/src/lib.rstests/support/reborn/harness.rstests/support_unit_tests.rs
26 tasks
ilblackdragon
force-pushed
the
feat/4644-image-vision
branch
from
June 15, 2026 05:41
68ce4ad to
7299152
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@crates/ironclaw_loop_support/src/lib.rs`:
- Around line 1211-1218: The `encode_image_parts` method is being called
unconditionally in two places, causing unnecessary filesystem reads and base64
encoding work for text-only models that cannot process images. Gate the
`encode_image_parts` calls at lines 1211-1218 and 1372-1381 with a conditional
check on the resolved model capability to determine if it supports vision. Only
call `encode_image_parts` when the model has vision capability enabled, allowing
text-only model routes to skip the expensive read and encode operations
entirely.
- Around line 1027-1033: The error handling branch for the Err case in the
attachment reading code around line 1027-1033 swallows the error from
read_attachment_bytes and continues, which needs to be explicitly marked as
intentional. Add a `// silent-ok: <reason>` inline comment above or adjacent to
the Err handler to document why it is acceptable to skip image attachments that
cannot be read, explaining that the model can proceed without unavailable
attachments rather than failing entirely.
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 2473-2487: The attachment_read_port field in
DefaultPlannedRuntimeParts is only wired when local_runtime is Some via the
map() call, leaving it None in production runtime. This causes multimodal image
parts to be dropped for vision-capable models. Fix this by providing a
production-scoped attachment reader when local_runtime is None, or implement a
fail-closed behavior that raises an error when attachments are present but no
reader can be provided. This aligns with the coding guideline that production
and migration-dry-run profiles must fail closed on missing required handles,
preventing silent degradation of behavior.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 09907c1f-0c63-44d1-86a7-f856ff5ea7f9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (24)
crates/ironclaw_loop_support/Cargo.tomlcrates/ironclaw_loop_support/src/lib.rscrates/ironclaw_loop_support/src/prompt_context_budget.rscrates/ironclaw_loop_support/src/system_inference.rscrates/ironclaw_loop_support/tests/thread_loop_support_contract.rscrates/ironclaw_product_workflow/tests/inbound_turn_contract.rscrates/ironclaw_product_workflow/tests/support/planned_agent_loop.rscrates/ironclaw_reborn/src/loop_driver_host.rscrates/ironclaw_reborn/src/loop_driver_host/model_gateway.rscrates/ironclaw_reborn/src/model_gateway.rscrates/ironclaw_reborn/src/runtime.rscrates/ironclaw_reborn/tests/llm_gateway.rscrates/ironclaw_reborn/tests/loop_driver_host.rscrates/ironclaw_reborn_composition/src/attachment_landing.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rscrates/ironclaw_reborn_composition/tests/product_live_adapters.rscrates/ironclaw_threads/src/attachment_context.rscrates/ironclaw_threads/src/contract.rscrates/ironclaw_threads/src/filesystem_service.rscrates/ironclaw_threads/src/in_memory.rscrates/ironclaw_threads/src/lib.rstests/support/reborn/harness.rstests/support_unit_tests.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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 `@crates/ironclaw_loop_support/src/lib.rs`:
- Around line 1211-1218: The `encode_image_parts` method is being called
unconditionally in two places, causing unnecessary filesystem reads and base64
encoding work for text-only models that cannot process images. Gate the
`encode_image_parts` calls at lines 1211-1218 and 1372-1381 with a conditional
check on the resolved model capability to determine if it supports vision. Only
call `encode_image_parts` when the model has vision capability enabled, allowing
text-only model routes to skip the expensive read and encode operations
entirely.
- Around line 1027-1033: The error handling branch for the Err case in the
attachment reading code around line 1027-1033 swallows the error from
read_attachment_bytes and continues, which needs to be explicitly marked as
intentional. Add a `// silent-ok: <reason>` inline comment above or adjacent to
the Err handler to document why it is acceptable to skip image attachments that
cannot be read, explaining that the model can proceed without unavailable
attachments rather than failing entirely.
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 2473-2487: The attachment_read_port field in
DefaultPlannedRuntimeParts is only wired when local_runtime is Some via the
map() call, leaving it None in production runtime. This causes multimodal image
parts to be dropped for vision-capable models. Fix this by providing a
production-scoped attachment reader when local_runtime is None, or implement a
fail-closed behavior that raises an error when attachments are present but no
reader can be provided. This aligns with the coding guideline that production
and migration-dry-run profiles must fail closed on missing required handles,
preventing silent degradation of behavior.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 09907c1f-0c63-44d1-86a7-f856ff5ea7f9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (24)
crates/ironclaw_loop_support/Cargo.tomlcrates/ironclaw_loop_support/src/lib.rscrates/ironclaw_loop_support/src/prompt_context_budget.rscrates/ironclaw_loop_support/src/system_inference.rscrates/ironclaw_loop_support/tests/thread_loop_support_contract.rscrates/ironclaw_product_workflow/tests/inbound_turn_contract.rscrates/ironclaw_product_workflow/tests/support/planned_agent_loop.rscrates/ironclaw_reborn/src/loop_driver_host.rscrates/ironclaw_reborn/src/loop_driver_host/model_gateway.rscrates/ironclaw_reborn/src/model_gateway.rscrates/ironclaw_reborn/src/runtime.rscrates/ironclaw_reborn/tests/llm_gateway.rscrates/ironclaw_reborn/tests/loop_driver_host.rscrates/ironclaw_reborn_composition/src/attachment_landing.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rscrates/ironclaw_reborn_composition/tests/product_live_adapters.rscrates/ironclaw_threads/src/attachment_context.rscrates/ironclaw_threads/src/contract.rscrates/ironclaw_threads/src/filesystem_service.rscrates/ironclaw_threads/src/in_memory.rscrates/ironclaw_threads/src/lib.rstests/support/reborn/harness.rstests/support_unit_tests.rs
🛑 Comments failed to post (3)
crates/ironclaw_loop_support/src/lib.rs (2)
1027-1033: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Mark this intentional read fallback with an explicit
// silent-ok:annotation.This branch swallows
read_attachment_byteserrors and continues. That can be valid here, but it needs the required inline// silent-ok: <reason>marker at the swallow site.Suggested patch
Err(error) => { + // silent-ok: read_attachment_bytes is best-effort for multimodal enrichment; + // fallback `<attachments>` text pointer remains available to the model. tracing::debug!( storage_key = %attachment.storage_key, %error, "skipping image attachment that could not be read for the model" ); }As per coding guidelines: “Fail loud by default… When a fallback is genuinely acceptable, it must be justified inline with a
// silent-ok: <reason>comment naming the operation.”🤖 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 `@crates/ironclaw_loop_support/src/lib.rs` around lines 1027 - 1033, The error handling branch for the Err case in the attachment reading code around line 1027-1033 swallows the error from read_attachment_bytes and continues, which needs to be explicitly marked as intentional. Add a `// silent-ok: <reason>` inline comment above or adjacent to the Err handler to document why it is acceptable to skip image attachments that cannot be read, explaining that the model can proceed without unavailable attachments rather than failing entirely.Source: Coding guidelines
1211-1218:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUnconditional image reads are happening before model capability gating.
Lines 1211-1218 and 1372-1381 always call
encode_image_parts, so text-only model routes still incur filesystem reads + base64 work that the gateway later discards. This breaks the intended lazy capability-gated read path and adds avoidable latency/IO on every turn.Gate image encoding before these calls using the resolved model capability (vision vs text-only), so non-vision requests skip read/encode entirely.
Also applies to: 1372-1381
🤖 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 `@crates/ironclaw_loop_support/src/lib.rs` around lines 1211 - 1218, The `encode_image_parts` method is being called unconditionally in two places, causing unnecessary filesystem reads and base64 encoding work for text-only models that cannot process images. Gate the `encode_image_parts` calls at lines 1211-1218 and 1372-1381 with a conditional check on the resolved model capability to determine if it supports vision. Only call `encode_image_parts` when the model has vision capability enabled, allowing text-only model routes to skip the expensive read and encode operations entirely.crates/ironclaw_reborn_composition/src/runtime.rs (1)
2473-2487:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftProduction runtime currently drops multimodal image parts.
On Line 2481,
attachment_read_portis wired only vialocal_runtime.map(...). In the production path,local_runtimeisNone, so the port is never set; downstream, image encoding short-circuits when the port is absent (seecrates/ironclaw_loop_support/src/lib.rsLines 1008-1011). This leaves production requests text-only even for vision-capable models.Please wire a production-scoped attachment reader source (or fail closed when attachments are present but no reader can be provided) so profile changes do not silently degrade behavior.
As per coding guidelines, "Production and migration-dry-run profiles must fail closed on local-only or missing required handles."
🤖 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 `@crates/ironclaw_reborn_composition/src/runtime.rs` around lines 2473 - 2487, The attachment_read_port field in DefaultPlannedRuntimeParts is only wired when local_runtime is Some via the map() call, leaving it None in production runtime. This causes multimodal image parts to be dropped for vision-capable models. Fix this by providing a production-scoped attachment reader when local_runtime is None, or implement a fail-closed behavior that raises an error when attachments are present but no reader can be provided. This aligns with the coding guideline that production and migration-dry-run profiles must fail closed on missing required handles, preventing silent degradation of behavior.Source: Coding guidelines
ilblackdragon
added a commit
that referenced
this pull request
Jun 15, 2026
Inline base64 image_url parts on /v1/chat/completions now reach a vision-capable model instead of being flattened to [image omitted]. Native non-adapter path (the product-adapter envelope is contractually bytes-free, so inline image bytes cannot ride it): - product_workflow: DefaultInboundTurnService gains attachment landing, and DefaultProductWorkflow exposes submit_inbound_with_attachments(envelope, Vec<InboundAttachment>) — decoded bytes are a direct arg (never serialized into the envelope), threaded to accept_prepared_user_message which lands them via the existing ProjectScopedAttachmentLander and accepts with MessageContent::with_attachments. Shares one pipeline with the bytes-free submit_inbound (no duplicate dispatch). - openai_compat: content_parts decodes image_url 'data:' URLs (base64, MIME allowlist + magic-byte validation, 10 MiB/image cap); carried images are omitted from transcript text (the landed attachment renders its own pointer downstream), while remote/malformed/oversized/mismatched images keep the [image omitted] marker. Defines the OpenAiCompatInboundAttachmentSubmit port (the route crate must not depend on ironclaw_product_workflow — enforced by reborn_dependency_boundaries); the chat workflow calls it when images are present and the door is wired, else falls back to the bytes-free submit. Chat body cap raised to 14 MiB. - composition: wires ProjectScopedAttachmentLander into the inbound turn service and bridges the route-crate port to DefaultProductWorkflow via OpenAiCompatAttachmentSubmitAdapter. Reuses the #4871 downstream model path unchanged (storage_key -> read port -> ContentPart::ImageUrl, vision-gated). product_adapters envelope stays bytes-free. Tests: content_parts decode/validation; product_workflow caller-level landing (lands inline bytes before acceptance; missing lander fails closed); descriptors contract updated for the 14 MiB chat body cap; architecture dependency boundaries hold.
Member
Author
|
Addressed the review comments (pushed up to gemini — CodeRabbit — Copilot — three doc comments over-promised a read-time vision gate ( CodeRabbit — CodeRabbit — mark the read-failure skip as intentional: added a CodeRabbit — Also in this push (beyond the original review scope, requested separately): image forwarding for the remaining vision providers (Anthropic OAuth / Gemini OAuth / Bedrock), a trigger-thread attachment-scope fix + de-duplicated thread-history resolution, WebUI v2 image thumbnails, the blob→data-URL CSP fix, and click-to-preview for all attachment kinds. Each has tests. |
| React.useEffect(() => { | ||
| // The local data URL is already renderable; only a persisted image needs | ||
| // the authenticated byte fetch. | ||
| if (att.preview_url || !att.fetch_url) return undefined; |
| }; | ||
| }, [att.preview_url, att.fetch_url]); | ||
|
|
||
| if (resolvedUrl) { |
Comment on lines
+160
to
+162
| // The timeline carries refs only — bytes stay behind the project mount — | ||
| // so `preview_url` is null and the card renders from metadata. | ||
| // so `preview_url` is null and the card renders from metadata. A document | ||
| // gets no `fetch_url` (only landed images are worth a thumbnail fetch). |
Comment on lines
+48
to
+55
| /// Decode an inline base64 `data:` URL into its `(media_type, base64_data)` | ||
| /// parts (e.g. `"data:image/png;base64,AQIDBA=="` → | ||
| /// `("image/png", "AQIDBA==")`). Returns `None` for a non-`data:` URL or a | ||
| /// `data:` URL that is not base64-encoded — callers that only support inline | ||
| /// bytes (Anthropic, Gemini, Bedrock) use this to forward the image and skip | ||
| /// remote URLs they can't fetch. The single shared parser keeps every | ||
| /// provider adapter consistent with the `data:` URL the model gateway emits. | ||
| pub fn decode_data_url(&self) -> Option<(&str, &str)> { |
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 (3)
crates/ironclaw_loop_support/tests/thread_loop_support_contract.rs (1)
2879-2883:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale producer-side image comment to match raw-bytes contract.
Line 2881 still says the producer threads a base64 image part, but this test now validates raw
bytesforwarding and gateway-side encoding later.Suggested fix
-/// [`LoopAttachmentReadPort`] and threaded to the gateway as a base64 image -/// part. The consumer side (`convert_messages` -> `ContentPart::ImageUrl`) is +/// [`LoopAttachmentReadPort`] and threaded to the gateway as raw image bytes in +/// a typed image part. The consumer side (`convert_messages` -> `ContentPart::ImageUrl`) isAs per coding guidelines: "When you change behavior in a function, re-read its docstring and adjacent comments — update or delete them in the same change."
🤖 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 `@crates/ironclaw_loop_support/tests/thread_loop_support_contract.rs` around lines 2879 - 2883, The docstring comment describing the image-vision path for LoopAttachmentReadPort contains outdated information stating that the producer threads a base64 image part to the gateway, but the current test validates raw bytes forwarding instead with gateway-side encoding happening later. Update the comment to remove the reference to base64 encoding and clarify that raw bytes are forwarded by the producer, with the encoding happening on the gateway side. This change should be made in the same commit where the behavior was updated to align with current coding guidelines about maintaining documentation consistency with implementation.Source: Coding guidelines
crates/ironclaw_threads/src/attachment_context.rs (1)
27-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the vision-gate doc to match the actual read path.
Line 29 says bytes are read “only for a vision model”, but the downstream loop port reads when the attachment read port is wired and leaves the single authoritative vision gate to the gateway (
crates/ironclaw_loop_support/src/lib.rs:998-1052). This stale wording invites a producer-side gate that could silently drop images when routing snapshots diverge.Suggested doc fix
/// The image attachments a vision-capable model could view as multimodal parts: /// `kind == Image` with a landed `storage_key`. Only the reference is carried — -/// the bytes are read later (and only for a vision model), so a text-only model -/// pays nothing here. The textual pointer from [`augment_model_content`] stays -/// as the fallback either way. +/// the bytes are read later by the loop model port when attachment reading is +/// wired. The gateway is the single authoritative vision gate, so a text-only +/// model drops these parts and relies on the textual pointer from +/// [`augment_model_content`] as the fallback.As per coding guidelines, “When you change behavior in a function, re-read its docstring and adjacent comments — update or delete them in the same change.”
🤖 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 `@crates/ironclaw_threads/src/attachment_context.rs` around lines 27 - 31, The docstring for the image attachments documentation in the attachment_context module contains stale wording that claims bytes are read "only for a vision model". This is inaccurate because the actual read behavior depends on whether the attachment read port is wired, and the vision gate is now handled at the gateway level (in crates/ironclaw_loop_support/src/lib.rs at lines 998-1052), not at the producer side. Update the docstring to accurately reflect that the read timing depends on the downstream attachment read port wiring rather than being gated by vision capability, and clarify that the authoritative vision gate is maintained at the gateway. This prevents confusion about read timing and avoids inviting a producer-side gate that could silently drop images when routing snapshots diverge.Source: Coding guidelines
crates/ironclaw_llm/src/vision_models.rs (1)
46-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the new Claude patterns to the suggestion priority list.
is_vision_model()now recognizesclaude-sonnet-4-6, butsuggest_vision_model()still checks onlyclaude-3/claude-4before GPT. With["gpt-4o", "claude-sonnet-4-6"], this returns GPT despite the documented Claude-first priority.Suggested fix
let priorities: &[&str] = &[ + "claude-opus-", + "claude-sonnet-", + "claude-haiku-", + "claude-fable-", "claude-3", "claude-4", "gpt-4o",Add the matching regression case too:
#[test] fn suggests_current_generation_claude_before_gpt4() { let models = vec![ "gpt-4o".to_string(), "claude-sonnet-4-6".to_string(), ]; assert_eq!(suggest_vision_model(&models), Some("claude-sonnet-4-6")); }As per coding guidelines, “Every bug fix must include a regression test.”
🤖 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 `@crates/ironclaw_llm/src/vision_models.rs` around lines 46 - 55, Update the priorities list in the suggest_vision_model function to include the new Claude patterns (such as claude-sonnet-4-6) that are now recognized by is_vision_model(), ensuring the priority order correctly favors current-generation Claude models over GPT models. Additionally, add a regression test following the suggested fix in the comment that verifies suggest_vision_model returns claude-sonnet-4-6 when both gpt-4o and claude-sonnet-4-6 are available, ensuring the documented Claude-first priority is maintained.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 `@crates/ironclaw_llm/src/anthropic_oauth.rs`:
- Around line 611-623: The issue is that the merge condition around line 667-672
can append tool results into any user Blocks, including multimodal blocks
created at line 611-623 that contain text and images. Tool-result coalescing
should only apply to blocks that are exclusively tool-result blocks. Modify the
merge condition at line 667-672 to restrict the tool result appending logic so
it only merges tool results into user Blocks that contain only tool-result
content blocks, preventing tool results from being incorrectly merged into
multimodal prompts with text and images.
In `@crates/ironclaw_llm/src/bedrock.rs`:
- Around line 336-344: The bedrock_image_format function performs case-sensitive
matching on the mime_type parameter, but MIME types are case-insensitive and can
be provided in various cases. Normalize the mime_type input to lowercase using
.to_ascii_lowercase() before performing the pattern matching in the match
statement, so that valid MIME type variants regardless of case (e.g.,
"image/PNG", "IMAGE/JPEG") are correctly recognized and mapped to their
corresponding ImageFormat values.
- Around line 349-362: The bedrock_image_block function contains two `.ok()?`
patterns (on the base64 decode operation and on the ImageBlock builder build
call) that silently discard errors without logging, making it impossible to
diagnose why images are being dropped. Replace each `.ok()?` with explicit error
handling: capture the error before returning None, and log the failure with
contextual information (such as "Failed to decode image data" or "Failed to
build image block") to aid debugging while still returning None to indicate
failure.
In `@crates/ironclaw_product_workflow/src/reborn_services.rs`:
- Around line 1318-1323: The storage_key parameter in the
InboundAttachmentReader trait's read method is currently a raw &str, which
allows arbitrary strings to pass through the public service boundary without
validation. Create or use an existing validated AttachmentStorageKey newtype and
replace the &str parameter with this typed identifier in the read method
signature. Then update all implementations of the InboundAttachmentReader trait
to use the new AttachmentStorageKey type instead of &str, ensuring the
identifier is properly validated at the boundary before being used by downstream
code.
- Around line 1940-1948: The early check for missing inbound_attachment_reader
at lines 1940-1948 returns 404 before determining if the attachment actually has
a storage_key, which masks composition faults. Refactor to resolve the
server-side attachment ref first, and only fail with a 503 status (instead of
404) if the attachment has a storage_key but the reader is missing. Apply the
same pattern to the second occurrence at lines 1982-1987. This ensures that
missing readers for actual persisted attachments fail loudly per the repo
invariant, rather than silently treating them as not found.
In `@crates/ironclaw_product_workflow/tests/reborn_services_contract.rs`:
- Around line 5486-5494: The RecordingAttachmentReader::read method currently
ignores the storage_key parameter (marked with underscore), which allows the
test to pass even if the service resolves the wrong attachment key. Modify the
method to record the storage_key by storing it (similar to how thread_scope is
stored in scopes), then add an assertion that validates the recorded storage_key
matches the expected attachment key to fully lock the contract for both scope
and key in trigger-thread reads.
In `@crates/ironclaw_webui_v2_static/src/router.rs`:
- Around line 411-418: The CSP assertions at lines 412 and 416 use substring
matching with `contains()` which allows CSP directives to pass the test even if
they are widened with additional sources, failing to act as a security boundary.
Replace the substring matching approach with proper parsing of the CSP
directives. Extract the actual tokens from the "media-src" directive and
"frame-src" directive separately, then assert that they contain exactly the
expected sources ('self' and data: for media-src; 'self' and blob: for
frame-src) with no additional sources present, ensuring the test fails closed on
any policy widening regression.
In `@crates/ironclaw_webui_v2_static/static/js/lib/api.js`:
- Around line 295-302: The fetchAttachmentBlob function sends an Authorization
bearer token via the Authorization header to any path without validating it is
same-origin first, creating a security vulnerability where tokens could be
leaked to off-origin URLs. Add same-origin URL validation logic before the fetch
call to ensure the path parameter is either a relative URL or points to the same
origin as the current page. Only set the Authorization header and proceed with
the fetch if the validation passes; reject off-origin absolute URLs before any
authentication headers are added.
In
`@crates/ironclaw_webui_v2_static/static/js/pages/chat/components/message-bubble.js`:
- Around line 20-45: The code currently fetches a data URL and renders an img
tag for all attachment types, but should only do this for image attachments. Add
a check to determine if the attachment is an image (by examining the filename
extension or MIME type available on the att object) and use this check to gate
both the fetchAttachmentDataUrl call in the useEffect hook (around line 25) and
the img rendering logic (around line 38). Non-image attachments should bypass
the fetch entirely and always show the file-icon fallback.
In `@crates/ironclaw_webui_v2/src/descriptors.rs`:
- Around line 291-303: The get_attachment_descriptor function incorrectly
classifies the attachment read route with AllowedEffectPath::ProjectionOnly, but
this endpoint performs actual product/workspace byte reads via the
read_attachment operation, not just projections. Change the AllowedEffectPath
parameter in the read_policy call within get_attachment_descriptor from
ProjectionOnly to the appropriate effect path variant that covers
product-workflow operations and filesystem/resource access to properly enforce
fail-closed ingress semantics.
---
Outside diff comments:
In `@crates/ironclaw_llm/src/vision_models.rs`:
- Around line 46-55: Update the priorities list in the suggest_vision_model
function to include the new Claude patterns (such as claude-sonnet-4-6) that are
now recognized by is_vision_model(), ensuring the priority order correctly
favors current-generation Claude models over GPT models. Additionally, add a
regression test following the suggested fix in the comment that verifies
suggest_vision_model returns claude-sonnet-4-6 when both gpt-4o and
claude-sonnet-4-6 are available, ensuring the documented Claude-first priority
is maintained.
In `@crates/ironclaw_loop_support/tests/thread_loop_support_contract.rs`:
- Around line 2879-2883: The docstring comment describing the image-vision path
for LoopAttachmentReadPort contains outdated information stating that the
producer threads a base64 image part to the gateway, but the current test
validates raw bytes forwarding instead with gateway-side encoding happening
later. Update the comment to remove the reference to base64 encoding and clarify
that raw bytes are forwarded by the producer, with the encoding happening on the
gateway side. This change should be made in the same commit where the behavior
was updated to align with current coding guidelines about maintaining
documentation consistency with implementation.
In `@crates/ironclaw_threads/src/attachment_context.rs`:
- Around line 27-31: The docstring for the image attachments documentation in
the attachment_context module contains stale wording that claims bytes are read
"only for a vision model". This is inaccurate because the actual read behavior
depends on whether the attachment read port is wired, and the vision gate is now
handled at the gateway level (in crates/ironclaw_loop_support/src/lib.rs at
lines 998-1052), not at the producer side. Update the docstring to accurately
reflect that the read timing depends on the downstream attachment read port
wiring rather than being gated by vision capability, and clarify that the
authoritative vision gate is maintained at the gateway. This prevents confusion
about read timing and avoids inviting a producer-side gate that could silently
drop images when routing snapshots diverge.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 5a938d80-5299-438a-9a07-2cffc2b0330a
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lockattachments/2026-06-15/26cfd2da-fb4e-4ccb-b4df-aa388053e215-1-23-sharing-signed-export.pngis excluded by!**/*.png
📒 Files selected for processing (36)
crates/ironclaw_llm/src/anthropic_oauth.rscrates/ironclaw_llm/src/bedrock.rscrates/ironclaw_llm/src/gemini_oauth.rscrates/ironclaw_llm/src/provider.rscrates/ironclaw_llm/src/vision_models.rscrates/ironclaw_loop_support/src/lib.rscrates/ironclaw_loop_support/tests/thread_loop_support_contract.rscrates/ironclaw_product_workflow/src/lib.rscrates/ironclaw_product_workflow/src/reborn_services.rscrates/ironclaw_product_workflow/src/reborn_services/types.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_reborn/Cargo.tomlcrates/ironclaw_reborn/src/model_gateway.rscrates/ironclaw_reborn/src/runtime.rscrates/ironclaw_reborn_composition/src/attachment_landing.rscrates/ironclaw_reborn_composition/src/webui.rscrates/ironclaw_threads/src/attachment_context.rscrates/ironclaw_threads/src/contract.rscrates/ironclaw_threads/src/filesystem_service.rscrates/ironclaw_threads/src/in_memory.rscrates/ironclaw_webui_v2/src/descriptors.rscrates/ironclaw_webui_v2/src/handlers.rscrates/ironclaw_webui_v2/src/lib.rscrates/ironclaw_webui_v2/src/router.rscrates/ironclaw_webui_v2/tests/webui_v2_descriptors_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rscrates/ironclaw_webui_v2_static/src/router.rscrates/ironclaw_webui_v2_static/static/js/lib/api.jscrates/ironclaw_webui_v2_static/static/js/lib/api.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/components/attachment-preview.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/components/message-bubble.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useHistory.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/attachments.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/attachments.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/history-messages.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/history-messages.test.mjs
Comment on lines
+1318
to
+1323
| pub trait InboundAttachmentReader: Send + Sync { | ||
| async fn read( | ||
| &self, | ||
| thread_scope: &ThreadScope, | ||
| storage_key: &str, | ||
| ) -> Result<Vec<u8>, RebornServicesError>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Type the attachment storage key before exposing this port.
storage_key is the authority-bearing identifier the host reader uses to locate bytes. Keeping it as &str in this public read port lets downstream code pass arbitrary strings; introduce/use a validated AttachmentStorageKey newtype, or pass a server-resolved attachment type with a typed key, before it crosses the workspace boundary. Repo invariant: strong identifier types at public/service boundaries. As per coding guidelines, Use newtypes for identifiers ... instead of raw String, &str, or uuid::Uuid.
🤖 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 `@crates/ironclaw_product_workflow/src/reborn_services.rs` around lines 1318 -
1323, The storage_key parameter in the InboundAttachmentReader trait's read
method is currently a raw &str, which allows arbitrary strings to pass through
the public service boundary without validation. Create or use an existing
validated AttachmentStorageKey newtype and replace the &str parameter with this
typed identifier in the read method signature. Then update all implementations
of the InboundAttachmentReader trait to use the new AttachmentStorageKey type
instead of &str, ensuring the identifier is properly validated at the boundary
before being used by downstream code.
Source: Coding guidelines
Member
Author
|
Addressed the re-review (pushed
Skipped with reason: typing |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_webui_v2_static/static/js/lib/api.test.mjs (1)
130-146:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore
URL.createObjectURLafter the test.Line 130 mutates a global, but Line 145 deletes it instead of restoring the previous value. This can leak state across tests and cause order-dependent failures.
Suggested fix
- globalThis.URL.createObjectURL = () => { - throw new Error("blob: URLs violate the SPA CSP img-src 'self' data:"); - }; - globalThis.FileReader = class { - readAsDataURL() { - this.result = "data:image/png;base64,AQIDBA=="; - if (this.onload) this.onload(); - } - }; - - const url = await fetchAttachmentDataUrl( - attachmentUrl({ threadId: "t1", messageId: "m1", attachmentId: "a1" }), - ); - - assert.ok(url.startsWith("data:"), `expected a data URL, got ${url}`); - delete globalThis.URL.createObjectURL; + const previousCreateObjectURL = globalThis.URL.createObjectURL; + try { + globalThis.URL.createObjectURL = () => { + throw new Error("blob: URLs violate the SPA CSP img-src 'self' data:"); + }; + globalThis.FileReader = class { + readAsDataURL() { + this.result = "data:image/png;base64,AQIDBA=="; + if (this.onload) this.onload(); + } + }; + + const url = await fetchAttachmentDataUrl( + attachmentUrl({ threadId: "t1", messageId: "m1", attachmentId: "a1" }), + ); + + assert.ok(url.startsWith("data:"), `expected a data URL, got ${url}`); + } finally { + if (previousCreateObjectURL) { + globalThis.URL.createObjectURL = previousCreateObjectURL; + } else { + delete globalThis.URL.createObjectURL; + } + }🤖 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 `@crates/ironclaw_webui_v2_static/static/js/lib/api.test.mjs` around lines 130 - 146, The test is mutating globalThis.URL.createObjectURL at the start but only deleting it rather than restoring the original value, which can leak state across tests. Before overwriting globalThis.URL.createObjectURL with the test mock, save the original value (which may be undefined). Then after the fetchAttachmentDataUrl call completes, restore the original value instead of deleting the property, ensuring proper cleanup and preventing test pollution.
🤖 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.
Outside diff comments:
In `@crates/ironclaw_webui_v2_static/static/js/lib/api.test.mjs`:
- Around line 130-146: The test is mutating globalThis.URL.createObjectURL at
the start but only deleting it rather than restoring the original value, which
can leak state across tests. Before overwriting globalThis.URL.createObjectURL
with the test mock, save the original value (which may be undefined). Then after
the fetchAttachmentDataUrl call completes, restore the original value instead of
deleting the property, ensuring proper cleanup and preventing test pollution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 02ce5887-7a48-4b2f-9352-6f13d9395057
📒 Files selected for processing (11)
crates/ironclaw_llm/src/anthropic_oauth.rscrates/ironclaw_llm/src/bedrock.rscrates/ironclaw_product_workflow/src/reborn_services.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_webui_v2/src/descriptors.rscrates/ironclaw_webui_v2/tests/webui_v2_descriptors_contract.rscrates/ironclaw_webui_v2_static/src/router.rscrates/ironclaw_webui_v2_static/static/js/lib/api.jscrates/ironclaw_webui_v2_static/static/js/lib/api.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/components/message-bubble.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/history-messages.test.mjs
Clicking an attachment chip now opens a focused preview modal. Each kind renders in a CSP-allowed way (classifier `attachmentPreviewMode`): - image → inline <img> (data URL; img-src 'self' data:) - audio/video → inline player (data URL; media-src 'self' data:) - pdf → inline <iframe> (blob URL; frame-src 'self' blob:) - text/JSON/CSV/XML → fetched text in a <pre> (capped, with a truncation note) - other binary → metadata panel; a Download action is offered in every mode. CSP: the SPA document shell gains `media-src 'self' data:` and `frame-src 'self' blob:` (deliberately narrow — locked by the spa_document_csp_allowlist_is_locked test so they can't widen to */data: frames; object-src stays 'none'). Plumbing: - api.js: `fetchAttachmentBlob` is the shared auth-fetch primitive; `blobToDataUrl` and the existing `fetchAttachmentDataUrl` build on it. The modal fetches bytes once and derives the per-mode representation, revoking the object URL on close. - history-messages: every landed attachment (not just images) now carries a `fetch_url`, so any kind can be previewed/downloaded. - message-bubble: the chip becomes a button (when it has bytes to show) that opens `AttachmentPreviewModal`; non-previewable optimistic rows stay static. Tests: `attachmentPreviewMode` mode mapping; landed non-image gets a `fetch_url`; CSP lock-test extended for the new directives.
…teway-side Review (Copilot): the doc claimed the turn layer reads attachment bytes "only when the active model can actually accept images", but the read is unconditional when a read port is wired — the vision/non-vision gate lives in the model gateway, not at read time. Reword to describe the gateway-side gate so the contract doesn't over-promise. (Companion lib.rs docs were already corrected.)
…hardening) Round-2 bot review of the pushed branch: - message-bubble: gate the thumbnail fetch+render to image kind. `fetch_url` was broadened to every landed attachment (for click-to-preview), which made the thumbnail fetch a PDF/text and render it as a broken <img>. Non-images keep the file icon. (Copilot, CodeRabbit) - bedrock: normalize MIME to lowercase before format matching; replace the `.ok()?` drops in `bedrock_image_block` with logged `// silent-ok` branches so a malformed/unsupported inline image is diagnosable, not silently gone. (CodeRabbit) - anthropic_oauth: restrict tool-result coalescing to user blocks that are *only* tool_result blocks, so a tool result can't be folded into a multimodal (text+image) user prompt. (CodeRabbit) - reborn_services `read_attachment`: resolve the attachment ref first; if it landed (has a storage_key) but no reader is wired, return a sanitized 503 (composition fault) instead of a 404 that makes real bytes look absent. (CodeRabbit — fail loud) - webui_v2 descriptor: the attachment-bytes route reads workspace-backed bytes, so classify it `ProductWorkflow`, not `ProjectionOnly` (fail-closed ingress). (CodeRabbit) - api.js `fetchAttachmentBlob`: reject off-origin URLs before attaching the bearer (token sink). (CodeRabbit) - CSP lock test: assert exact per-directive source lists (media-src/frame-src/ img-src) instead of substrings, so a widened directive fails. (CodeRabbit) - tests: trigger-thread regression now records + asserts the storage_key passed to the reader (scope + key); reworded a misleading history-messages test comment. (CodeRabbit, Copilot) Skipped: typing `storage_key` as an `AttachmentStorageKey` newtype at the read port — `storage_key` is `String` across the whole attachment subsystem (`AttachmentRef`, lander, loop port), and the reader already re-scopes it through the `MountView`/`ScopedPath` authority so it can't escape the project scope; a newtype just here would be inconsistent and is a separate cross-cutting refactor. PR-description "no ironclaw_llm changes" note: will update the PR body.
…he dir `attachments/2026-06-15/26cfd2da-…-sharing-signed-export.png` is a runtime artifact: the WebChat v2 attachment lander wrote an uploaded image under the project workspace while `serve` ran from the repo root, and a `git add -A` in 45e4d67 swept it in. It is not source. Remove it and add `/attachments/` to .gitignore so local-dev uploads can't be committed again.
…attachment cost Round-3 review (Copilot, CodeRabbit): - `attachmentUrl` now throws if threadId/messageId/attachmentId is missing, rather than building a `.../undefined/...` path that would later carry the bearer via `fetchAttachmentBlob`. history-messages guards all three parts so a malformed record yields a plain card (no fetch) instead of throwing mid- projection. (Copilot, api.js) - api.test.mjs: save/restore `URL.createObjectURL` (try/finally) instead of deleting it, so the stub can't leak global state across tests. Added a `attachmentUrl fails fast` test. (CodeRabbit, outside-diff) - read_attachment: documented why it loads full thread history (O(messages)) — the cost equals the timeline load already incurred when the thread is open and each attachment is browser-cached, and a single-message fast path would need a new scope-validated "load one message record by id" service method (`load_context_messages` only projects image refs). Left as a follow-up rather than widening the thread-service contract. (Copilot) JS regression test in api.test.mjs (browser-JS behavior, not Rust). [skip-regression-check]
ilblackdragon
force-pushed
the
feat/4644-image-vision
branch
from
June 15, 2026 22:55
dda915d to
5183484
Compare
Comment on lines
+190
to
+193
| headers.insert( | ||
| header::CACHE_CONTROL, | ||
| HeaderValue::from_static("private, max-age=300"), | ||
| ); |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/ironclaw_loop_support/src/lib.rs (1)
1459-1460:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the gateway doc: it is no longer text-only.
HostManagedModelMessagenow carries transientimage_parts, so this public trait comment contradicts the new contract. As per coding guidelines, “When you change behavior in a function, re-read its docstring and adjacent comments — update or delete them in the same change.”Suggested fix
-/// Host-managed text-only model gateway. Implementations own provider selection, -/// profile policy, retry/circuit behavior, and sanitization. +/// Host-managed model gateway. Implementations own provider selection, +/// profile policy, retry/circuit behavior, multimodal shaping, and sanitization.🤖 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 `@crates/ironclaw_loop_support/src/lib.rs` around lines 1459 - 1460, The docstring comment for the host-managed model gateway incorrectly describes it as "text-only model gateway" when `HostManagedModelMessage` now carries transient `image_parts`, contradicting the actual behavior. Update the documentation comment to remove the "text-only" constraint and accurately reflect that the gateway now supports image content in addition to text, ensuring the docstring aligns with the current contract of the trait/interface.Source: Coding guidelines
crates/ironclaw_reborn/src/model_gateway.rs (1)
184-193:⚠️ Potential issue | 🟠 MajorAdd
attachment_read_portfield toThreadBackedLoopModelGatewayand wire it instream_model().The struct at line 130 is missing the
attachment_read_portfield entirely, so.stream_model()cannot pass it toThreadBackedLoopModelPort. This blocks multimodal forwarding when using this gateway directly. The separate production path (LoopDriverHost→ThreadResolvingLoopModelGateway) is correctly wired, butThreadBackedLoopModelGatewayshould match for consistency and usability.Add the field to the struct, add a
with_attachment_read_port()builder method, and propagate it when constructing the port (lines 184–193), mirroring the pattern already inThreadResolvingLoopModelGateway(loop_driver_host/model_gateway.rs, lines 31, 67).🤖 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 `@crates/ironclaw_reborn/src/model_gateway.rs` around lines 184 - 193, The ThreadBackedLoopModelGateway struct is missing the attachment_read_port field required for multimodal forwarding support. Add the attachment_read_port field to the struct definition in ThreadBackedLoopModelGateway, implement a with_attachment_read_port() builder method following the same pattern as existing builder methods like with_instruction_materialization_store(), and then pass the attachment_read_port when constructing ThreadBackedLoopModelPort in the stream_model() method (around lines 184-193) to match the implementation pattern already present in ThreadResolvingLoopModelGateway.Source: Coding guidelines
crates/ironclaw_product_workflow/src/reborn_services.rs (1)
3121-3155:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the stale fallback-only doc block from the new resolver.
This doc now attaches to
resolve_thread_history_for_caller, but Lines 3121-3138 still describe only the old fallback and citelist_automations; the implementation first does the owner-scoped history read and delegates authorization toresolve_run_thread_scope.Proposed doc cleanup
- /// Fallback timeline fetch for automation-trigger threads. - /// - /// Automation-trigger threads are created under the trigger creator's - /// scope, not the caller's session scope. The normal user-scoped - /// `list_thread_history` therefore always misses them. This fallback is - /// only reached when the user-scoped lookup returned `UnknownThread` or - /// `ThreadScopeMismatch`. - /// - /// Authorization: the thread_id must appear in at least one `recent_run` - /// for an automation returned by `list_automations` for this caller. That - /// is the same authorization check the Automations list endpoint applies, - /// so no new trust boundary is introduced. Authorization is revalidated on - /// every call — no caching. - /// - /// On authorization success, the history is loaded with the trigger-owned - /// scope. On authorization failure (thread not in any of the caller's - /// automation runs), the `original_not_found_error` is returned so the - /// response is indistinguishable from a genuinely absent thread. /// Resolve a caller-visible thread's history together with the thread scope @@ - /// one of the caller's automations (`list_automations` applies the same - /// authorization), the history is re-fetched under the trigger-owned scope. + /// one of the caller's automations (`resolve_run_thread_scope` performs the + /// caller-scoped lookup), the history is re-fetched under the trigger-owned scope.Repo invariant: preserve tenant/user/agent/project/thread scope at authority boundaries. As per coding guidelines,
When you change behavior in a function, re-read its docstring and adjacent comments — update or delete them in the same change.🤖 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 `@crates/ironclaw_product_workflow/src/reborn_services.rs` around lines 3121 - 3155, The docstring for the `resolve_thread_history_for_caller` function (lines 3121-3138) describes outdated fallback-only behavior and references `list_automations` for authorization, but the actual implementation first performs an owner-scoped history read and delegates authorization to `resolve_run_thread_scope`. Replace the stale documentation block with an updated docstring that accurately describes the current implementation: that it performs an owner-scoped history read first and uses `resolve_run_thread_scope` for authorization checks, rather than the fallback-centric description and `list_automations` reference. Ensure the updated docstring maintains clarity about scope boundaries and authorization flow to align with the actual code behavior.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 `@crates/ironclaw_llm/src/vision_models.rs`:
- Around line 18-21: The Claude prioritization logic at lines 46-55 only checks
for specific version strings like "claude-3" and "claude-4", but it fails to
recognize newer Claude tier-first models like "claude-sonnet-" and
"claude-haiku-" shown at lines 18-21. Update the prioritization checks at lines
46-55 to match against all the Claude model prefixes defined at lines 18-21,
ensuring that any model starting with these prefixes is recognized as a
prioritized Claude model. This will restore the intended invariant where any
Claude model takes priority over GPT-4, Gemini, and other models.
In `@crates/ironclaw_loop_support/src/lib.rs`:
- Around line 1039-1047: The tracing::debug! call in the error handler is
logging the raw attachment.storage_key value, which can expose sensitive
filesystem paths or backend information. Replace the storage_key field in the
debug log with a sanitized alternative such as an index number or a
hashed/digest representation of the storage key, while retaining the error field
which contains safe diagnostic information about why the attachment could not be
read.
In `@crates/ironclaw_loop_support/tests/thread_loop_support_contract.rs`:
- Around line 2869-2874: The StubImageReader mock in read_attachment_bytes
method ignores the _scope parameter and only records the storage_key, which
means tests cannot verify that the model port calls the reader with the correct
tenant/user/project scope. Modify the mock to capture and record both the scope
and storage_key arguments (remove the underscore prefix from _scope and add it
to whatever data structure tracks the reads), then update the assertion in the
model_port_reads_image_attachment_bytes_into_model_image_parts test to verify
both the expected scope and storage_key match what was recorded. Apply this same
fix at the other location indicated in the comment (lines 2941-2945) where this
pattern also appears.
In `@crates/ironclaw_product_workflow/src/reborn_services.rs`:
- Around line 997-1056: The docstrings for trace_credits and
authorize_trace_hold describe user-only scoping and zero-on-unreadable-state
behavior, but the actual implementation uses tenant-scoped keys and maps read
failures to fail-loud errors (internal_from). Update the docstring for
trace_credits to clarify that the scope is derived from both tenant_id and
user_id (not user id only), and to describe that read failures surface as
sanitized 500 errors per fail-loud semantics (not silent zero responses).
Similarly, update the docstring for authorize_trace_hold to clarify that the
scope is tenant-scoped. Ensure the contract text reflects the actual fail-loud
behavior so that implementations and test fakes do not mistakenly copy the old
silent-fallback semantics.
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 1545-1547: The shutdown sequence in the runtime.rs file has a race
condition where trace_flush_worker.shutdown() is called before
worker_cancel.cancel(), creating a window where new trace items could be
enqueued after the flusher stops but before the producer is cancelled, causing
silent loss of tail trace events. Reverse the shutdown order by calling
self.worker_cancel.cancel() first to stop the producer, then call
self.trace_flush_worker.shutdown().await afterwards to ensure no new trace items
are enqueued while the flusher is shutting down.
---
Outside diff comments:
In `@crates/ironclaw_loop_support/src/lib.rs`:
- Around line 1459-1460: The docstring comment for the host-managed model
gateway incorrectly describes it as "text-only model gateway" when
`HostManagedModelMessage` now carries transient `image_parts`, contradicting the
actual behavior. Update the documentation comment to remove the "text-only"
constraint and accurately reflect that the gateway now supports image content in
addition to text, ensuring the docstring aligns with the current contract of the
trait/interface.
In `@crates/ironclaw_product_workflow/src/reborn_services.rs`:
- Around line 3121-3155: The docstring for the
`resolve_thread_history_for_caller` function (lines 3121-3138) describes
outdated fallback-only behavior and references `list_automations` for
authorization, but the actual implementation first performs an owner-scoped
history read and delegates authorization to `resolve_run_thread_scope`. Replace
the stale documentation block with an updated docstring that accurately
describes the current implementation: that it performs an owner-scoped history
read first and uses `resolve_run_thread_scope` for authorization checks, rather
than the fallback-centric description and `list_automations` reference. Ensure
the updated docstring maintains clarity about scope boundaries and authorization
flow to align with the actual code behavior.
In `@crates/ironclaw_reborn/src/model_gateway.rs`:
- Around line 184-193: The ThreadBackedLoopModelGateway struct is missing the
attachment_read_port field required for multimodal forwarding support. Add the
attachment_read_port field to the struct definition in
ThreadBackedLoopModelGateway, implement a with_attachment_read_port() builder
method following the same pattern as existing builder methods like
with_instruction_materialization_store(), and then pass the attachment_read_port
when constructing ThreadBackedLoopModelPort in the stream_model() method (around
lines 184-193) to match the implementation pattern already present in
ThreadResolvingLoopModelGateway.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 7d07f4a6-49f4-493b-8813-d491dbcb4a29
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (51)
.gitignorecrates/ironclaw_llm/src/anthropic_oauth.rscrates/ironclaw_llm/src/bedrock.rscrates/ironclaw_llm/src/gemini_oauth.rscrates/ironclaw_llm/src/provider.rscrates/ironclaw_llm/src/vision_models.rscrates/ironclaw_loop_support/src/lib.rscrates/ironclaw_loop_support/src/prompt_context_budget.rscrates/ironclaw_loop_support/src/system_inference.rscrates/ironclaw_loop_support/tests/thread_loop_support_contract.rscrates/ironclaw_product_workflow/src/lib.rscrates/ironclaw_product_workflow/src/reborn_services.rscrates/ironclaw_product_workflow/src/reborn_services/types.rscrates/ironclaw_product_workflow/tests/inbound_turn_contract.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_product_workflow/tests/support/planned_agent_loop.rscrates/ironclaw_reborn/Cargo.tomlcrates/ironclaw_reborn/src/loop_driver_host.rscrates/ironclaw_reborn/src/loop_driver_host/model_gateway.rscrates/ironclaw_reborn/src/model_gateway.rscrates/ironclaw_reborn/src/runtime.rscrates/ironclaw_reborn/tests/llm_gateway.rscrates/ironclaw_reborn/tests/loop_driver_host.rscrates/ironclaw_reborn_composition/src/attachment_landing.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rscrates/ironclaw_reborn_composition/src/webui.rscrates/ironclaw_reborn_composition/tests/product_live_adapters.rscrates/ironclaw_threads/src/attachment_context.rscrates/ironclaw_threads/src/contract.rscrates/ironclaw_threads/src/filesystem_service.rscrates/ironclaw_threads/src/in_memory.rscrates/ironclaw_threads/src/lib.rscrates/ironclaw_webui_v2/src/descriptors.rscrates/ironclaw_webui_v2/src/handlers.rscrates/ironclaw_webui_v2/src/lib.rscrates/ironclaw_webui_v2/src/router.rscrates/ironclaw_webui_v2/tests/webui_v2_descriptors_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rscrates/ironclaw_webui_v2_static/src/router.rscrates/ironclaw_webui_v2_static/static/js/lib/api.jscrates/ironclaw_webui_v2_static/static/js/lib/api.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/components/attachment-preview.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/components/message-bubble.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useHistory.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/attachments.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/attachments.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/history-messages.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/history-messages.test.mjstests/support/reborn/harness.rstests/support_unit_tests.rs
💤 Files with no reviewable changes (25)
- tests/support/reborn/harness.rs
- crates/ironclaw_reborn_composition/tests/product_live_adapters.rs
- tests/support_unit_tests.rs
- crates/ironclaw_reborn_composition/src/webui.rs
- crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useHistory.js
- crates/ironclaw_webui_v2/src/lib.rs
- crates/ironclaw_webui_v2/tests/webui_v2_descriptors_contract.rs
- crates/ironclaw_webui_v2_static/static/js/lib/api.test.mjs
- crates/ironclaw_webui_v2/src/descriptors.rs
- crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/attachments.test.mjs
- crates/ironclaw_threads/src/contract.rs
- crates/ironclaw_threads/src/in_memory.rs
- crates/ironclaw_webui_v2/src/handlers.rs
- crates/ironclaw_threads/src/filesystem_service.rs
- crates/ironclaw_threads/src/attachment_context.rs
- crates/ironclaw_threads/src/lib.rs
- crates/ironclaw_webui_v2/src/router.rs
- crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/attachments.js
- crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/history-messages.js
- crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/history-messages.test.mjs
- crates/ironclaw_webui_v2_static/static/js/pages/chat/components/message-bubble.js
- crates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rs
- crates/ironclaw_webui_v2_static/src/router.rs
- crates/ironclaw_webui_v2_static/static/js/lib/api.js
- crates/ironclaw_webui_v2_static/static/js/pages/chat/components/attachment-preview.js
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/ironclaw_loop_support/src/lib.rs (1)
1459-1460:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the gateway doc: it is no longer text-only.
HostManagedModelMessagenow carries transientimage_parts, so this public trait comment contradicts the new contract. As per coding guidelines, “When you change behavior in a function, re-read its docstring and adjacent comments — update or delete them in the same change.”Suggested fix
-/// Host-managed text-only model gateway. Implementations own provider selection, -/// profile policy, retry/circuit behavior, and sanitization. +/// Host-managed model gateway. Implementations own provider selection, +/// profile policy, retry/circuit behavior, multimodal shaping, and sanitization.🤖 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 `@crates/ironclaw_loop_support/src/lib.rs` around lines 1459 - 1460, The docstring comment for the host-managed model gateway incorrectly describes it as "text-only model gateway" when `HostManagedModelMessage` now carries transient `image_parts`, contradicting the actual behavior. Update the documentation comment to remove the "text-only" constraint and accurately reflect that the gateway now supports image content in addition to text, ensuring the docstring aligns with the current contract of the trait/interface.Source: Coding guidelines
crates/ironclaw_reborn/src/model_gateway.rs (1)
184-193:⚠️ Potential issue | 🟠 MajorAdd
attachment_read_portfield toThreadBackedLoopModelGatewayand wire it instream_model().The struct at line 130 is missing the
attachment_read_portfield entirely, so.stream_model()cannot pass it toThreadBackedLoopModelPort. This blocks multimodal forwarding when using this gateway directly. The separate production path (LoopDriverHost→ThreadResolvingLoopModelGateway) is correctly wired, butThreadBackedLoopModelGatewayshould match for consistency and usability.Add the field to the struct, add a
with_attachment_read_port()builder method, and propagate it when constructing the port (lines 184–193), mirroring the pattern already inThreadResolvingLoopModelGateway(loop_driver_host/model_gateway.rs, lines 31, 67).🤖 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 `@crates/ironclaw_reborn/src/model_gateway.rs` around lines 184 - 193, The ThreadBackedLoopModelGateway struct is missing the attachment_read_port field required for multimodal forwarding support. Add the attachment_read_port field to the struct definition in ThreadBackedLoopModelGateway, implement a with_attachment_read_port() builder method following the same pattern as existing builder methods like with_instruction_materialization_store(), and then pass the attachment_read_port when constructing ThreadBackedLoopModelPort in the stream_model() method (around lines 184-193) to match the implementation pattern already present in ThreadResolvingLoopModelGateway.Source: Coding guidelines
crates/ironclaw_product_workflow/src/reborn_services.rs (1)
3121-3155:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the stale fallback-only doc block from the new resolver.
This doc now attaches to
resolve_thread_history_for_caller, but Lines 3121-3138 still describe only the old fallback and citelist_automations; the implementation first does the owner-scoped history read and delegates authorization toresolve_run_thread_scope.Proposed doc cleanup
- /// Fallback timeline fetch for automation-trigger threads. - /// - /// Automation-trigger threads are created under the trigger creator's - /// scope, not the caller's session scope. The normal user-scoped - /// `list_thread_history` therefore always misses them. This fallback is - /// only reached when the user-scoped lookup returned `UnknownThread` or - /// `ThreadScopeMismatch`. - /// - /// Authorization: the thread_id must appear in at least one `recent_run` - /// for an automation returned by `list_automations` for this caller. That - /// is the same authorization check the Automations list endpoint applies, - /// so no new trust boundary is introduced. Authorization is revalidated on - /// every call — no caching. - /// - /// On authorization success, the history is loaded with the trigger-owned - /// scope. On authorization failure (thread not in any of the caller's - /// automation runs), the `original_not_found_error` is returned so the - /// response is indistinguishable from a genuinely absent thread. /// Resolve a caller-visible thread's history together with the thread scope @@ - /// one of the caller's automations (`list_automations` applies the same - /// authorization), the history is re-fetched under the trigger-owned scope. + /// one of the caller's automations (`resolve_run_thread_scope` performs the + /// caller-scoped lookup), the history is re-fetched under the trigger-owned scope.Repo invariant: preserve tenant/user/agent/project/thread scope at authority boundaries. As per coding guidelines,
When you change behavior in a function, re-read its docstring and adjacent comments — update or delete them in the same change.🤖 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 `@crates/ironclaw_product_workflow/src/reborn_services.rs` around lines 3121 - 3155, The docstring for the `resolve_thread_history_for_caller` function (lines 3121-3138) describes outdated fallback-only behavior and references `list_automations` for authorization, but the actual implementation first performs an owner-scoped history read and delegates authorization to `resolve_run_thread_scope`. Replace the stale documentation block with an updated docstring that accurately describes the current implementation: that it performs an owner-scoped history read first and uses `resolve_run_thread_scope` for authorization checks, rather than the fallback-centric description and `list_automations` reference. Ensure the updated docstring maintains clarity about scope boundaries and authorization flow to align with the actual code behavior.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 `@crates/ironclaw_llm/src/vision_models.rs`:
- Around line 18-21: The Claude prioritization logic at lines 46-55 only checks
for specific version strings like "claude-3" and "claude-4", but it fails to
recognize newer Claude tier-first models like "claude-sonnet-" and
"claude-haiku-" shown at lines 18-21. Update the prioritization checks at lines
46-55 to match against all the Claude model prefixes defined at lines 18-21,
ensuring that any model starting with these prefixes is recognized as a
prioritized Claude model. This will restore the intended invariant where any
Claude model takes priority over GPT-4, Gemini, and other models.
In `@crates/ironclaw_loop_support/src/lib.rs`:
- Around line 1039-1047: The tracing::debug! call in the error handler is
logging the raw attachment.storage_key value, which can expose sensitive
filesystem paths or backend information. Replace the storage_key field in the
debug log with a sanitized alternative such as an index number or a
hashed/digest representation of the storage key, while retaining the error field
which contains safe diagnostic information about why the attachment could not be
read.
In `@crates/ironclaw_loop_support/tests/thread_loop_support_contract.rs`:
- Around line 2869-2874: The StubImageReader mock in read_attachment_bytes
method ignores the _scope parameter and only records the storage_key, which
means tests cannot verify that the model port calls the reader with the correct
tenant/user/project scope. Modify the mock to capture and record both the scope
and storage_key arguments (remove the underscore prefix from _scope and add it
to whatever data structure tracks the reads), then update the assertion in the
model_port_reads_image_attachment_bytes_into_model_image_parts test to verify
both the expected scope and storage_key match what was recorded. Apply this same
fix at the other location indicated in the comment (lines 2941-2945) where this
pattern also appears.
In `@crates/ironclaw_product_workflow/src/reborn_services.rs`:
- Around line 997-1056: The docstrings for trace_credits and
authorize_trace_hold describe user-only scoping and zero-on-unreadable-state
behavior, but the actual implementation uses tenant-scoped keys and maps read
failures to fail-loud errors (internal_from). Update the docstring for
trace_credits to clarify that the scope is derived from both tenant_id and
user_id (not user id only), and to describe that read failures surface as
sanitized 500 errors per fail-loud semantics (not silent zero responses).
Similarly, update the docstring for authorize_trace_hold to clarify that the
scope is tenant-scoped. Ensure the contract text reflects the actual fail-loud
behavior so that implementations and test fakes do not mistakenly copy the old
silent-fallback semantics.
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 1545-1547: The shutdown sequence in the runtime.rs file has a race
condition where trace_flush_worker.shutdown() is called before
worker_cancel.cancel(), creating a window where new trace items could be
enqueued after the flusher stops but before the producer is cancelled, causing
silent loss of tail trace events. Reverse the shutdown order by calling
self.worker_cancel.cancel() first to stop the producer, then call
self.trace_flush_worker.shutdown().await afterwards to ensure no new trace items
are enqueued while the flusher is shutting down.
---
Outside diff comments:
In `@crates/ironclaw_loop_support/src/lib.rs`:
- Around line 1459-1460: The docstring comment for the host-managed model
gateway incorrectly describes it as "text-only model gateway" when
`HostManagedModelMessage` now carries transient `image_parts`, contradicting the
actual behavior. Update the documentation comment to remove the "text-only"
constraint and accurately reflect that the gateway now supports image content in
addition to text, ensuring the docstring aligns with the current contract of the
trait/interface.
In `@crates/ironclaw_product_workflow/src/reborn_services.rs`:
- Around line 3121-3155: The docstring for the
`resolve_thread_history_for_caller` function (lines 3121-3138) describes
outdated fallback-only behavior and references `list_automations` for
authorization, but the actual implementation first performs an owner-scoped
history read and delegates authorization to `resolve_run_thread_scope`. Replace
the stale documentation block with an updated docstring that accurately
describes the current implementation: that it performs an owner-scoped history
read first and uses `resolve_run_thread_scope` for authorization checks, rather
than the fallback-centric description and `list_automations` reference. Ensure
the updated docstring maintains clarity about scope boundaries and authorization
flow to align with the actual code behavior.
In `@crates/ironclaw_reborn/src/model_gateway.rs`:
- Around line 184-193: The ThreadBackedLoopModelGateway struct is missing the
attachment_read_port field required for multimodal forwarding support. Add the
attachment_read_port field to the struct definition in
ThreadBackedLoopModelGateway, implement a with_attachment_read_port() builder
method following the same pattern as existing builder methods like
with_instruction_materialization_store(), and then pass the attachment_read_port
when constructing ThreadBackedLoopModelPort in the stream_model() method (around
lines 184-193) to match the implementation pattern already present in
ThreadResolvingLoopModelGateway.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 7d07f4a6-49f4-493b-8813-d491dbcb4a29
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (51)
.gitignorecrates/ironclaw_llm/src/anthropic_oauth.rscrates/ironclaw_llm/src/bedrock.rscrates/ironclaw_llm/src/gemini_oauth.rscrates/ironclaw_llm/src/provider.rscrates/ironclaw_llm/src/vision_models.rscrates/ironclaw_loop_support/src/lib.rscrates/ironclaw_loop_support/src/prompt_context_budget.rscrates/ironclaw_loop_support/src/system_inference.rscrates/ironclaw_loop_support/tests/thread_loop_support_contract.rscrates/ironclaw_product_workflow/src/lib.rscrates/ironclaw_product_workflow/src/reborn_services.rscrates/ironclaw_product_workflow/src/reborn_services/types.rscrates/ironclaw_product_workflow/tests/inbound_turn_contract.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_product_workflow/tests/support/planned_agent_loop.rscrates/ironclaw_reborn/Cargo.tomlcrates/ironclaw_reborn/src/loop_driver_host.rscrates/ironclaw_reborn/src/loop_driver_host/model_gateway.rscrates/ironclaw_reborn/src/model_gateway.rscrates/ironclaw_reborn/src/runtime.rscrates/ironclaw_reborn/tests/llm_gateway.rscrates/ironclaw_reborn/tests/loop_driver_host.rscrates/ironclaw_reborn_composition/src/attachment_landing.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rscrates/ironclaw_reborn_composition/src/webui.rscrates/ironclaw_reborn_composition/tests/product_live_adapters.rscrates/ironclaw_threads/src/attachment_context.rscrates/ironclaw_threads/src/contract.rscrates/ironclaw_threads/src/filesystem_service.rscrates/ironclaw_threads/src/in_memory.rscrates/ironclaw_threads/src/lib.rscrates/ironclaw_webui_v2/src/descriptors.rscrates/ironclaw_webui_v2/src/handlers.rscrates/ironclaw_webui_v2/src/lib.rscrates/ironclaw_webui_v2/src/router.rscrates/ironclaw_webui_v2/tests/webui_v2_descriptors_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rscrates/ironclaw_webui_v2_static/src/router.rscrates/ironclaw_webui_v2_static/static/js/lib/api.jscrates/ironclaw_webui_v2_static/static/js/lib/api.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/components/attachment-preview.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/components/message-bubble.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useHistory.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/attachments.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/attachments.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/history-messages.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/history-messages.test.mjstests/support/reborn/harness.rstests/support_unit_tests.rs
💤 Files with no reviewable changes (25)
- tests/support/reborn/harness.rs
- crates/ironclaw_reborn_composition/tests/product_live_adapters.rs
- tests/support_unit_tests.rs
- crates/ironclaw_reborn_composition/src/webui.rs
- crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useHistory.js
- crates/ironclaw_webui_v2/src/lib.rs
- crates/ironclaw_webui_v2/tests/webui_v2_descriptors_contract.rs
- crates/ironclaw_webui_v2_static/static/js/lib/api.test.mjs
- crates/ironclaw_webui_v2/src/descriptors.rs
- crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/attachments.test.mjs
- crates/ironclaw_threads/src/contract.rs
- crates/ironclaw_threads/src/in_memory.rs
- crates/ironclaw_webui_v2/src/handlers.rs
- crates/ironclaw_threads/src/filesystem_service.rs
- crates/ironclaw_threads/src/attachment_context.rs
- crates/ironclaw_threads/src/lib.rs
- crates/ironclaw_webui_v2/src/router.rs
- crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/attachments.js
- crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/history-messages.js
- crates/ironclaw_webui_v2_static/static/js/pages/chat/lib/history-messages.test.mjs
- crates/ironclaw_webui_v2_static/static/js/pages/chat/components/message-bubble.js
- crates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rs
- crates/ironclaw_webui_v2_static/src/router.rs
- crates/ironclaw_webui_v2_static/static/js/lib/api.js
- crates/ironclaw_webui_v2_static/static/js/pages/chat/components/attachment-preview.js
🛑 Comments failed to post (5)
crates/ironclaw_llm/src/vision_models.rs (1)
18-21:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winClaude-first selection is broken for tier-first Claude IDs.
Line 44 documents the invariant
Claude > GPT-4 > Gemini > others, but lines 46-55 only prioritizeclaude-3/claude-4. With models like["gpt-4o", "claude-sonnet-4-6"], this now returns GPT first even though tier-first Claude is vision-capable.Suggested fix
pub fn suggest_vision_model(models: &[String]) -> Option<&str> { let priorities: &[&str] = &[ "claude-3", "claude-4", + "claude-opus-", + "claude-sonnet-", + "claude-haiku-", + "claude-fable-", "gpt-4o", "gpt-4-turbo", "gpt-4-vision", "gemini", "llava", "pixtral", ];#[test] + fn suggests_tier_first_claude_before_gpt() { + let models = vec!["gpt-4o".to_string(), "claude-sonnet-4-6".to_string()]; + assert_eq!(suggest_vision_model(&models), Some("claude-sonnet-4-6")); + } + + #[test] fn returns_none_when_no_vision_models() {Also applies to: 46-55
🤖 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 `@crates/ironclaw_llm/src/vision_models.rs` around lines 18 - 21, The Claude prioritization logic at lines 46-55 only checks for specific version strings like "claude-3" and "claude-4", but it fails to recognize newer Claude tier-first models like "claude-sonnet-" and "claude-haiku-" shown at lines 18-21. Update the prioritization checks at lines 46-55 to match against all the Claude model prefixes defined at lines 18-21, ensuring that any model starting with these prefixes is recognized as a prioritized Claude model. This will restore the intended invariant where any Claude model takes priority over GPT-4, Gemini, and other models.crates/ironclaw_loop_support/src/lib.rs (1)
1039-1047:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not log raw attachment storage keys.
Line 1044 emits
attachment.storage_key; the supplied contract test shows these can be/workspace/...attachment paths. Log an index or digest instead, while keeping the sanitized error cause. As per coding guidelines, “Do not expose raw secrets, backend paths, private URLs, transport internals, raw SQL/backend errors, or unredacted runtime/user content across public surfaces.”Suggested fix
- for attachment in attachments { + for (attachment_index, attachment) in attachments.iter().enumerate() { match port .read_attachment_bytes(&scope, &attachment.storage_key) .await @@ Err(error) => { tracing::debug!( - storage_key = %attachment.storage_key, + attachment_index, %error, "skipping image attachment that could not be read for the model" ); }🤖 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 `@crates/ironclaw_loop_support/src/lib.rs` around lines 1039 - 1047, The tracing::debug! call in the error handler is logging the raw attachment.storage_key value, which can expose sensitive filesystem paths or backend information. Replace the storage_key field in the debug log with a sanitized alternative such as an index number or a hashed/digest representation of the storage key, while retaining the error field which contains safe diagnostic information about why the attachment could not be read.Source: Coding guidelines
crates/ironclaw_loop_support/tests/thread_loop_support_contract.rs (1)
2869-2874:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCapture and assert the read
scopein the attachment-reader mock
StubImageReaderrecords onlystorage_key;_scopeis ignored, so this test can still pass if the model port calls the reader with the wrong tenant/user/project scope.Suggested fix
struct StubImageReader { bytes: Vec<u8>, - reads: Mutex<Vec<String>>, + reads: Mutex<Vec<(ResourceScope, String)>>, } #[async_trait] impl LoopAttachmentReadPort for StubImageReader { async fn read_attachment_bytes( &self, - _scope: &ResourceScope, + scope: &ResourceScope, storage_key: &str, ) -> Result<Vec<u8>, LoopAttachmentReadError> { - self.reads.lock().unwrap().push(storage_key.to_string()); + self.reads + .lock() + .unwrap() + .push((scope.clone(), storage_key.to_string())); Ok(self.bytes.clone()) } }Then assert both the expected scope and storage key in
model_port_reads_image_attachment_bytes_into_model_image_parts.As per coding guidelines:
**/*test*.rs: “When mocking a multi-arg runtime API, the mock must capture every argument the production caller passes.”Also applies to: 2941-2945
🤖 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 `@crates/ironclaw_loop_support/tests/thread_loop_support_contract.rs` around lines 2869 - 2874, The StubImageReader mock in read_attachment_bytes method ignores the _scope parameter and only records the storage_key, which means tests cannot verify that the model port calls the reader with the correct tenant/user/project scope. Modify the mock to capture and record both the scope and storage_key arguments (remove the underscore prefix from _scope and add it to whatever data structure tracks the reads), then update the assertion in the model_port_reads_image_attachment_bytes_into_model_image_parts test to verify both the expected scope and storage_key match what was recorded. Apply this same fix at the other location indicated in the comment (lines 2941-2945) where this pattern also appears.Source: Coding guidelines
crates/ironclaw_product_workflow/src/reborn_services.rs (1)
997-1056:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the Trace Commons docs with the fail-loud implementation.
Lines 1000-1003 and 1030-1032 still describe user-only scope / zero-on-unreadable behavior, but the code tenant-scopes the key and maps read failures to
internal_from. Keep the implementation; fix the contract text so fakes and downstream impls do not copy the old silent-fallback semantics.Proposed doc fix
- /// The trace scope derives from the caller's user id only — never - /// from request input. Missing or unreadable contributor-local - /// state is the normal "not enrolled / nothing submitted yet" - /// zero response, never an error. The aggregates are a local view + /// The trace scope derives from the caller's tenant + user id only — never + /// from request input. Missing contributor-local state is the normal + /// "not enrolled / nothing submitted yet" zero response; read failures + /// surface as sanitized internal errors. The aggregates are a local view @@ - /// (promote-as-is). The scope is always the authenticated caller's user - /// id; the submission id from the request path is never authority to + /// (promote-as-is). The scope is always the authenticated caller's tenant + /// + user id; the submission id from the request path is never authority toRepo invariant: Fail loud. As per coding guidelines,
When you change behavior in a function, re-read its docstring and adjacent comments — update or delete them in the same change.🤖 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 `@crates/ironclaw_product_workflow/src/reborn_services.rs` around lines 997 - 1056, The docstrings for trace_credits and authorize_trace_hold describe user-only scoping and zero-on-unreadable-state behavior, but the actual implementation uses tenant-scoped keys and maps read failures to fail-loud errors (internal_from). Update the docstring for trace_credits to clarify that the scope is derived from both tenant_id and user_id (not user id only), and to describe that read failures surface as sanitized 500 errors per fail-loud semantics (not silent zero responses). Similarly, update the docstring for authorize_trace_hold to clarify that the scope is tenant-scoped. Ensure the contract text reflects the actual fail-loud behavior so that implementations and test fakes do not mistakenly copy the old silent-fallback semantics.Source: Coding guidelines
crates/ironclaw_reborn_composition/src/runtime.rs (1)
1545-1547:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winShutdown order can silently drop tail trace events.
Line 1545 stops the trace flusher before Line 1546 cancels the turn-runner producer. That leaves a window where new trace items can be enqueued with no active flusher.
Suggested fix
- self.trace_flush_worker.shutdown().await; self.worker_cancel.cancel(); if let Some(projection) = self.budget_event_projection { projection.shutdown().await; } if let Err(error) = self.worker_handle.await { if error.is_panic() { tracing::error!(%error, "reborn worker task panicked during shutdown"); } else { tracing::warn!(%error, "reborn worker task was cancelled during shutdown"); } } + self.trace_flush_worker.shutdown().await;As per coding guidelines, the named invariant “Fail loud” requires avoiding silent-failure paths; this ordering creates a silent tail-loss path for trace capture on shutdown.
🤖 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 `@crates/ironclaw_reborn_composition/src/runtime.rs` around lines 1545 - 1547, The shutdown sequence in the runtime.rs file has a race condition where trace_flush_worker.shutdown() is called before worker_cancel.cancel(), creating a window where new trace items could be enqueued after the flusher stops but before the producer is cancelled, causing silent loss of tail trace events. Reverse the shutdown order by calling self.worker_cancel.cancel() first to stop the producer, then call self.trace_flush_worker.shutdown().await afterwards to ensure no new trace items are enqueued while the flusher is shutting down.Source: Coding guidelines
ilblackdragon
disabled auto-merge
June 15, 2026 23:37
This was referenced Jun 16, 2026
ilblackdragon
added a commit
that referenced
this pull request
Jun 16, 2026
main merged #4871 (image attachment support for vision-capable models), which supersedes this branch's outbound image-read path with a cleaner design: read_image_parts returns raw bytes, the gateway owns base64/data-URL encoding and the single vision gate (no read-time gate), and ContextMessage construction is consolidated in from_transcript_message. Resolution: adopt main's outbound design wholesale (took theirs for loop_support, model_gateway, threads, attachment_landing, runtime), and dropped this session's now-redundant read-gate work (model_vision_capable, route_accepts_images, encode_image_parts) plus its orphaned tests. Kept this branch's genuinely-unique contribution — the openai-compat inbound door (submit_inbound_with_attachments / accept_*_and_attachments / with_inbound_attachments) that lets the OpenAI-compatible surface land inline images through main's InboundAttachmentLander. inbound_turn.rs was reconciled as a true 3-way merge: main's DeferredBusy->RejectedBusy semantics + this branch's attachment door, both test sets retained. Builds: product_workflow, openai_compat, and composition compile against merged main.
ilblackdragon
added a commit
that referenced
this pull request
Jun 16, 2026
- CI (cargo-deny): ignore RUSTSEC-2026-0182 (wasmtime WASIp1 fd_renumber leak), same surface justification as the already-ignored 0149 — the WASM sandbox grants guests no WASIp1 fd capabilities, so it's unreachable. `cargo deny check advisories` → ok locally. - workflow: fail closed when submit_inbound_with_attachments gets inline bytes on a non-UserMessage payload — dispatch_payload only lands them for UserMessage, so other kinds silently dropped user files. Now rejected (400) before binding/ledger. Regression test added at the contract tier. - loop_support: drop the now-unused `base64` dependency (main's #4871 moved base64/data-URL formatting into the gateway; loop_support carries raw bytes). - content_parts: annotate the base64 `.ok()?` with `// silent-ok:` — a malformed inline image from untrusted input is expected and falls back to the bounded `[image omitted]` marker, not a server error.
ilblackdragon
added a commit
that referenced
this pull request
Jun 16, 2026
* feat(threads): carry image attachment refs on ContextMessage (image-vision step 1)
Foundation for sending image attachments to vision-capable models. The
transcript already holds AttachmentRef { kind: Image, mime_type, storage_key };
this surfaces the image ones structurally on the model-visible ContextMessage
(new ContextImageAttachment { mime_type, storage_key }) instead of only the
textual <attachments> pointer. Cheap metadata — no bytes are read here; the turn
layer will read+encode them later, and only for a vision-capable model. The text
pointer remains the fallback for text-only models. Populated at both context-read
paths (window + by-id) in the in-memory and filesystem stores.
* feat(reborn): translate image attachments into multimodal parts (image-vision step 2)
The model gateway now renders image attachments as real multimodal content for
the provider. HostManagedModelMessage carries encoded image parts
(HostManagedModelImagePart { mime_type, data_base64 }); convert_messages turns a
User message's parts into `ContentPart::ImageUrl` base64 `data:` URLs via
`ChatMessage::user_with_parts` (text rides in `content`, images in
`content_parts` — the provider adapters prepend the text). Text-only user
messages are unchanged. The parts are still empty until step 3 reads+encodes
bytes for vision models; this is the gateway-side translation, unit-tested for
both the image and text-only paths.
* feat(reborn): vision gate + attachment read port (image-vision step 3a)
The model gateway now only attaches image parts for a vision-capable model
(`is_vision_model(provider_model_id)`); a text-only model keeps just the text
(the transcript's `<attachments>` pointer still serves it). And loop_support
gains a `LoopAttachmentReadPort` (read bytes by scope + storage_key) plus a
`with_attachment_read_port` builder: when wired, `resolve_model_messages` reads
each model-visible message's image attachments through the port and
base64-encodes them into `HostManagedModelImagePart`s. Until the port is injected
(step 3b) it stays None, so behavior is unchanged. Read failures are logged and
skipped, never failing the turn. Unit tests cover the gate (vision vs non-vision).
* feat(reborn): wire the attachment read port end-to-end (image-vision step 3b)
Inject the concrete attachment reader so the multimodal path is now live for the
WebChat v2 loop. `ProjectScopedAttachmentReader` reads landed bytes back through
the project workspace filesystem (bounded, re-scoped through the MountView
authority — never a host path) and implements `LoopAttachmentReadPort`. It is
threaded factory → gateway struct → model port (mirroring the
`skill_context_source` plumbing), and `build_reborn_runtime` constructs it from
the local runtime's workspace filesystem and sets it on
`DefaultPlannedRuntimeParts`. When no local runtime is composed it stays `None`
(images remain the textual pointer). With this, an image attached to a
vision-capable model is read, base64-encoded, and sent as a `ContentPart::ImageUrl`.
* docs(threads): image multimodal path is implemented (image-vision step 5)
The attachment_context doc no longer describes the vision path as a separate
future path — it now points at the implemented multimodal flow (model port
reads bytes back, gateway sends ContentPart::ImageUrl) and frames the textual
pointer as the text-only-model fallback.
* test(4644): cover image-vision producer read path + fix reader error taxonomy
Add producer-side integration coverage for the image-vision feature, which
previously had only consumer-side unit tests (convert_messages):
- thread_loop_support_contract: drive the model port with a landed image
attachment + a stub LoopAttachmentReadPort, asserting the resolved
HostManagedModelMessage carries the base64-encoded bytes as an image part
(test-through-the-caller: the read port gates a side effect with the model
port wrapper between).
- attachment_landing: round-trip (land -> read back), not-found, and oversized
reader tests.
The oversized test surfaced a bug: read_bytes_bounded returns Ok(None) only
for oversized files (a missing file is Err(NotFound)), so the reader mislabeled
oversized attachments as NotFound and missing ones as a generic Backend error.
Fix the mapping: Ok(None) -> Backend("exceeds limit"), Err(NotFound) ->
NotFound, Err(PermissionDenied) -> Forbidden.
* test(4644): set attachment_read_port in reborn parity harness
The image-vision field added to DefaultPlannedRuntimeParts was not threaded
into the root reborn parity test harness, breaking compilation of every
reborn_*_parity integration target (E0063) and the --all-targets clippy/test
jobs. Set it to None (these parity tests don't exercise the image path).
* feat(openai-compat): vision support for inline images (#4644)
Inline base64 image_url parts on /v1/chat/completions now reach a
vision-capable model instead of being flattened to [image omitted].
Native non-adapter path (the product-adapter envelope is contractually
bytes-free, so inline image bytes cannot ride it):
- product_workflow: DefaultInboundTurnService gains attachment landing, and
DefaultProductWorkflow exposes submit_inbound_with_attachments(envelope,
Vec<InboundAttachment>) — decoded bytes are a direct arg (never serialized
into the envelope), threaded to accept_prepared_user_message which lands them
via the existing ProjectScopedAttachmentLander and accepts with
MessageContent::with_attachments. Shares one pipeline with the bytes-free
submit_inbound (no duplicate dispatch).
- openai_compat: content_parts decodes image_url 'data:' URLs (base64, MIME
allowlist + magic-byte validation, 10 MiB/image cap); carried images are
omitted from transcript text (the landed attachment renders its own pointer
downstream), while remote/malformed/oversized/mismatched images keep the
[image omitted] marker. Defines the OpenAiCompatInboundAttachmentSubmit port
(the route crate must not depend on ironclaw_product_workflow — enforced by
reborn_dependency_boundaries); the chat workflow calls it when images are
present and the door is wired, else falls back to the bytes-free submit. Chat
body cap raised to 14 MiB.
- composition: wires ProjectScopedAttachmentLander into the inbound turn service
and bridges the route-crate port to DefaultProductWorkflow via
OpenAiCompatAttachmentSubmitAdapter.
Reuses the #4871 downstream model path unchanged (storage_key -> read port ->
ContentPart::ImageUrl, vision-gated). product_adapters envelope stays bytes-free.
Tests: content_parts decode/validation; product_workflow caller-level landing
(lands inline bytes before acceptance; missing lander fails closed); descriptors
contract updated for the 14 MiB chat body cap; architecture dependency
boundaries hold.
* refactor(4644): gate image read via gateway capability + harden attachment paths
Code-quality follow-ups to the inline-image vision feature:
- Ask the gateway whether its model accepts images, instead of importing the
LLM model catalog into the loop-wiring layer. Adds a defaulted
`HostManagedModelGateway::route_accepts_images`; the two LLM-backed gateways
override it from the exact model they send to (active model / route snapshot),
so the loop's image read-gate matches `convert_messages`'s send-gate. The
bridge file (`loop_driver_host`) no longer depends on `ironclaw_llm` and the
feature-gated helper is gone.
This also fixes a latent bug: the live `serve` runtime wires no model-route
resolver, so `resolved_model_route` is `None` — the earlier read-gate keyed on
it (`unwrap_or(false)`) and silently disabled vision in production. Keying on
the gateway's `active_model_name()` (always present) restores it.
- Fail loud on dropped attachments. The InboundTurnService trait default now
rejects a turn carrying inline bytes instead of silently dropping them.
- Collapse the duplicated 14 MiB chat-body cap to a single MAX_CHAT_BODY_BYTES
source of truth in descriptors.rs, used by both the ingress body_limit and the
in-workflow body check.
Adds caller-level coverage: gateway image-capability seam (vision/text models),
the non-vision read short-circuit, and the fail-loud default. Builds clean with
and without root-llm-provider; clippy clean.
* fix(openai-compat): address review — base64 whitespace + unwired-door image fallback
Two PR review findings on the inline-image decode path:
- HIGH: when no attachment-submit door is wired, a successfully decoded inline
image was dropped from the transcript text (omitted as a "carried" attachment)
AND not carried (no door) — the image vanished with no trace. Thread an
`enable_attachments` flag (= door wired) into content_value_to_text_and_images;
when false, image_url parts fall back to the `[image omitted]` marker like any
other unsupported part, so an image is never silently lost.
- MEDIUM: standard MIME base64 encoders wrap lines, so a data: URL payload can
contain newlines/spaces the strict decoder rejects, dropping a valid image to
`[image omitted]`. Strip ASCII whitespace before decoding.
Regression tests: marker fallback when attachments disabled, and decode through
wrapping whitespace.
* fix(4644): green CI + address Copilot/CodeRabbit review
- CI (cargo-deny): ignore RUSTSEC-2026-0182 (wasmtime WASIp1 fd_renumber leak),
same surface justification as the already-ignored 0149 — the WASM sandbox
grants guests no WASIp1 fd capabilities, so it's unreachable. `cargo deny
check advisories` → ok locally.
- workflow: fail closed when submit_inbound_with_attachments gets inline bytes
on a non-UserMessage payload — dispatch_payload only lands them for
UserMessage, so other kinds silently dropped user files. Now rejected (400)
before binding/ledger. Regression test added at the contract tier.
- loop_support: drop the now-unused `base64` dependency (main's #4871 moved
base64/data-URL formatting into the gateway; loop_support carries raw bytes).
- content_parts: annotate the base64 `.ok()?` with `// silent-ok:` — a
malformed inline image from untrusted input is expected and falls back to the
bounded `[image omitted]` marker, not a server error.
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…nearai#4644) (nearai#4871) * feat(threads): carry image attachment refs on ContextMessage (image-vision step 1) Foundation for sending image attachments to vision-capable models. The transcript already holds AttachmentRef { kind: Image, mime_type, storage_key }; this surfaces the image ones structurally on the model-visible ContextMessage (new ContextImageAttachment { mime_type, storage_key }) instead of only the textual <attachments> pointer. Cheap metadata — no bytes are read here; the turn layer will read+encode them later, and only for a vision-capable model. The text pointer remains the fallback for text-only models. Populated at both context-read paths (window + by-id) in the in-memory and filesystem stores. * feat(reborn): translate image attachments into multimodal parts (image-vision step 2) The model gateway now renders image attachments as real multimodal content for the provider. HostManagedModelMessage carries encoded image parts (HostManagedModelImagePart { mime_type, data_base64 }); convert_messages turns a User message's parts into `ContentPart::ImageUrl` base64 `data:` URLs via `ChatMessage::user_with_parts` (text rides in `content`, images in `content_parts` — the provider adapters prepend the text). Text-only user messages are unchanged. The parts are still empty until step 3 reads+encodes bytes for vision models; this is the gateway-side translation, unit-tested for both the image and text-only paths. * feat(reborn): vision gate + attachment read port (image-vision step 3a) The model gateway now only attaches image parts for a vision-capable model (`is_vision_model(provider_model_id)`); a text-only model keeps just the text (the transcript's `<attachments>` pointer still serves it). And loop_support gains a `LoopAttachmentReadPort` (read bytes by scope + storage_key) plus a `with_attachment_read_port` builder: when wired, `resolve_model_messages` reads each model-visible message's image attachments through the port and base64-encodes them into `HostManagedModelImagePart`s. Until the port is injected (step 3b) it stays None, so behavior is unchanged. Read failures are logged and skipped, never failing the turn. Unit tests cover the gate (vision vs non-vision). * feat(reborn): wire the attachment read port end-to-end (image-vision step 3b) Inject the concrete attachment reader so the multimodal path is now live for the WebChat v2 loop. `ProjectScopedAttachmentReader` reads landed bytes back through the project workspace filesystem (bounded, re-scoped through the MountView authority — never a host path) and implements `LoopAttachmentReadPort`. It is threaded factory → gateway struct → model port (mirroring the `skill_context_source` plumbing), and `build_reborn_runtime` constructs it from the local runtime's workspace filesystem and sets it on `DefaultPlannedRuntimeParts`. When no local runtime is composed it stays `None` (images remain the textual pointer). With this, an image attached to a vision-capable model is read, base64-encoded, and sent as a `ContentPart::ImageUrl`. * docs(threads): image multimodal path is implemented (image-vision step 5) The attachment_context doc no longer describes the vision path as a separate future path — it now points at the implemented multimodal flow (model port reads bytes back, gateway sends ContentPart::ImageUrl) and frames the textual pointer as the text-only-model fallback. * test(4644): cover image-vision producer read path + fix reader error taxonomy Add producer-side integration coverage for the image-vision feature, which previously had only consumer-side unit tests (convert_messages): - thread_loop_support_contract: drive the model port with a landed image attachment + a stub LoopAttachmentReadPort, asserting the resolved HostManagedModelMessage carries the base64-encoded bytes as an image part (test-through-the-caller: the read port gates a side effect with the model port wrapper between). - attachment_landing: round-trip (land -> read back), not-found, and oversized reader tests. The oversized test surfaced a bug: read_bytes_bounded returns Ok(None) only for oversized files (a missing file is Err(NotFound)), so the reader mislabeled oversized attachments as NotFound and missing ones as a generic Backend error. Fix the mapping: Ok(None) -> Backend("exceeds limit"), Err(NotFound) -> NotFound, Err(PermissionDenied) -> Forbidden. * test(4644): set attachment_read_port in reborn parity harness The image-vision field added to DefaultPlannedRuntimeParts was not threaded into the root reborn parity test harness, breaking compilation of every reborn_*_parity integration target (E0063) and the --all-targets clippy/test jobs. Set it to None (these parity tests don't exercise the image path). * fix(attachments): correct vision gate + tidy image-vision read path (review) Address code-quality review and bot comments on the image-attachment path: - Vision gate (blocker): is_vision_model missed the current tier-first Claude ids (`claude-opus-4-8`, `claude-sonnet-4-6`, Bedrock `anthropic.claude-*`), so every current-gen Claude model was mis-classified text-only and silently dropped image attachments. Added tier-first patterns + a regression test over the real production ids. - Layering: HostManagedModelImagePart now carries raw bytes, not base64; the gateway owns base64/`data:` URL formatting (image_data_url). Drops the base64 dependency from the neutral ironclaw_loop_support crate. Renamed the producer helper encode_image_parts -> read_image_parts to match. - Documented why the read is not gated on vision capability at the producer: the authoritative model id is model_override (resolved in the gateway from its routing policy) and can diverge from the run-context route snapshot the port holds, so a producer-side gate would risk silently dropping images. The single authoritative gate stays in convert_messages; the silent skip is annotated. - Decomposition: collapsed the 4-site ContextMessage projection (2 stores x 2 paths) into ContextMessage::from_transcript_message so attachment projection lives once and the two stores cannot drift. - LoopAttachmentReadError now impls std::error::Error. - Documented attachment_read_port optionality as deliberate (no workspace fs -> nothing to read -> degrade to text pointer), not a fail-closed gap. - Added unit tests for model_image_attachments. * feat(attachments): vision support across providers + WebUI v2 image thumbnails Two follow-on gaps from the image-attachment work: 1) Image reading now works on every vision-capable provider. The gateway already emitted ContentPart::ImageUrl for vision models, but three adapters silently dropped it: - anthropic_oauth: emits Anthropic `image` blocks (base64 source). - gemini_oauth: emits Gemini `inlineData` parts. - bedrock: emits Converse `ImageBlock` (supported formats only; others are skipped, keeping the text). Added a shared `ImageUrl::decode_data_url()` so every adapter parses the gateway's `data:` URL the same way. Each adapter has a unit test. (OpenAI Codex Responses API still has no image support upstream — unchanged.) 2) WebUI v2 renders thumbnails for persisted images. The timeline carries only attachment refs (no bytes), so a reloaded image showed a file card, not a thumbnail. Added: - GET /api/webchat/v2/threads/{thread_id}/messages/{message_id}/attachments/ {attachment_id} — serves landed bytes, scope derived from the authenticated caller, storage path resolved server-side, authoritative Content-Type + nosniff + short private cache. Keyed by (thread, message, attachment) because an attachment id is only unique within its message. - RebornServicesApi::read_attachment (default returns NotFound; only RebornServices overrides it, so the 10 stub impls are untouched) backed by a new InboundAttachmentReader port — the read counterpart of InboundAttachmentLander — implemented over the same project workspace mount and wired in build_webui_services. - Frontend: `<img>` can't send a bearer, so the bubble lazily fetches the bytes (authenticated) into a blob URL and revokes it on unmount; the timeline projection attaches a `fetch_url` to landed images. Contract tests cover the descriptor, the byte response through the real router, and the projection URL. * refactor(reborn): fix trigger-thread attachment scope + dedupe history resolution Self-review of the attachment-bytes path found a latent bug and a duplication: - Bug: `read_attachment` resolved the thread via the automation-trigger fallback (so a trigger-fired thread is found) but then read the bytes under the *caller's* session scope. Trigger threads live under the creator's scope, so the reader addressed the wrong project mount and would 404 for trigger-thread images — exactly the case the fallback exists to support. - Duplication: the scope-derivation + history-load + trigger-fallback block was copy-pasted between `get_timeline` and `read_attachment` (~25 lines of security-sensitive logic that could drift). Fix: `try_automation_trigger_timeline_fallback` now returns the resolved `ThreadScope` it already computes alongside the history, and a single `resolve_thread_history_for_caller` helper owns the primary-load + fallback. `get_timeline` ignores the scope; `read_attachment` reads bytes under it, so a trigger-thread thumbnail loads under the creator's mount. Regression test: `read_attachment_reads_trigger_thread_bytes_under_creator_scope` asserts the byte read is issued under the trigger creator's scope, not the caller's session scope (fails on the pre-fix code). * fix(webui-v2): render attachment thumbnails as data URLs, not blob URLs The persisted-image thumbnail fetched bytes into a `blob:` object URL, but the SPA's CSP is `img-src 'self' data:` — so `<img src=blob:…>` was refused ("violates ... img-src 'self' data:"). The optimistic compose-time preview already uses a `data:` URL (FileReader.readAsDataURL), which is why it rendered and the persisted one didn't. Match that convention instead of widening the CSP: `fetchAttachmentDataUrl` reads the fetched blob into a `data:` URL. This is CSP-compliant and also drops the object-URL revoke lifecycle (data URLs need none). Same-origin authenticated fetch is unchanged (allowed by `connect-src 'self'`). Regression test lives in api.test.mjs (a browser-JS bug, not Rust): it stubs `URL.createObjectURL` to throw and asserts `fetchAttachmentDataUrl` returns a `data:` URL — reverting to a blob URL fails the test. [skip-regression-check] (the hook only scans for Rust #[test]; the test is JS). * feat(webui-v2): click-to-preview modal for all attachment kinds Clicking an attachment chip now opens a focused preview modal. Each kind renders in a CSP-allowed way (classifier `attachmentPreviewMode`): - image → inline <img> (data URL; img-src 'self' data:) - audio/video → inline player (data URL; media-src 'self' data:) - pdf → inline <iframe> (blob URL; frame-src 'self' blob:) - text/JSON/CSV/XML → fetched text in a <pre> (capped, with a truncation note) - other binary → metadata panel; a Download action is offered in every mode. CSP: the SPA document shell gains `media-src 'self' data:` and `frame-src 'self' blob:` (deliberately narrow — locked by the spa_document_csp_allowlist_is_locked test so they can't widen to */data: frames; object-src stays 'none'). Plumbing: - api.js: `fetchAttachmentBlob` is the shared auth-fetch primitive; `blobToDataUrl` and the existing `fetchAttachmentDataUrl` build on it. The modal fetches bytes once and derives the per-mode representation, revoking the object URL on close. - history-messages: every landed attachment (not just images) now carries a `fetch_url`, so any kind can be previewed/downloaded. - message-bubble: the chip becomes a button (when it has bytes to show) that opens `AttachmentPreviewModal`; non-previewable optimistic rows stay static. Tests: `attachmentPreviewMode` mode mapping; landed non-image gets a `fetch_url`; CSP lock-test extended for the new directives. * docs(threads): correct ContextImageAttachment doc — vision gate is gateway-side Review (Copilot): the doc claimed the turn layer reads attachment bytes "only when the active model can actually accept images", but the read is unconditional when a read port is wired — the vision/non-vision gate lives in the model gateway, not at read time. Reword to describe the gateway-side gate so the contract doesn't over-promise. (Companion lib.rs docs were already corrected.) * fix(attachments): address PR re-review (thumbnail gating, fail-loud, hardening) Round-2 bot review of the pushed branch: - message-bubble: gate the thumbnail fetch+render to image kind. `fetch_url` was broadened to every landed attachment (for click-to-preview), which made the thumbnail fetch a PDF/text and render it as a broken <img>. Non-images keep the file icon. (Copilot, CodeRabbit) - bedrock: normalize MIME to lowercase before format matching; replace the `.ok()?` drops in `bedrock_image_block` with logged `// silent-ok` branches so a malformed/unsupported inline image is diagnosable, not silently gone. (CodeRabbit) - anthropic_oauth: restrict tool-result coalescing to user blocks that are *only* tool_result blocks, so a tool result can't be folded into a multimodal (text+image) user prompt. (CodeRabbit) - reborn_services `read_attachment`: resolve the attachment ref first; if it landed (has a storage_key) but no reader is wired, return a sanitized 503 (composition fault) instead of a 404 that makes real bytes look absent. (CodeRabbit — fail loud) - webui_v2 descriptor: the attachment-bytes route reads workspace-backed bytes, so classify it `ProductWorkflow`, not `ProjectionOnly` (fail-closed ingress). (CodeRabbit) - api.js `fetchAttachmentBlob`: reject off-origin URLs before attaching the bearer (token sink). (CodeRabbit) - CSP lock test: assert exact per-directive source lists (media-src/frame-src/ img-src) instead of substrings, so a widened directive fails. (CodeRabbit) - tests: trigger-thread regression now records + asserts the storage_key passed to the reader (scope + key); reworded a misleading history-messages test comment. (CodeRabbit, Copilot) Skipped: typing `storage_key` as an `AttachmentStorageKey` newtype at the read port — `storage_key` is `String` across the whole attachment subsystem (`AttachmentRef`, lander, loop port), and the reader already re-scopes it through the `MountView`/`ScopedPath` authority so it can't escape the project scope; a newtype just here would be inconsistent and is a separate cross-cutting refactor. PR-description "no ironclaw_llm changes" note: will update the PR body. * chore: remove accidentally-committed runtime attachment + gitignore the dir `attachments/2026-06-15/26cfd2da-…-sharing-signed-export.png` is a runtime artifact: the WebChat v2 attachment lander wrote an uploaded image under the project workspace while `serve` ran from the repo root, and a `git add -A` in 45e4d67 swept it in. It is not source. Remove it and add `/attachments/` to .gitignore so local-dev uploads can't be committed again. * fix(webui-v2): fail-fast attachmentUrl + test hygiene; document read_attachment cost Round-3 review (Copilot, CodeRabbit): - `attachmentUrl` now throws if threadId/messageId/attachmentId is missing, rather than building a `.../undefined/...` path that would later carry the bearer via `fetchAttachmentBlob`. history-messages guards all three parts so a malformed record yields a plain card (no fetch) instead of throwing mid- projection. (Copilot, api.js) - api.test.mjs: save/restore `URL.createObjectURL` (try/finally) instead of deleting it, so the stub can't leak global state across tests. Added a `attachmentUrl fails fast` test. (CodeRabbit, outside-diff) - read_attachment: documented why it loads full thread history (O(messages)) — the cost equals the timeline load already incurred when the thread is open and each attachment is browser-cached, and a single-message fast path would need a new scope-validated "load one message record by id" service method (`load_context_messages` only projects image refs). Left as a follow-up rather than widening the thread-service contract. (Copilot) JS regression test in api.test.mjs (browser-JS behavior, not Rust). [skip-regression-check]
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…earai#4902) * feat(threads): carry image attachment refs on ContextMessage (image-vision step 1) Foundation for sending image attachments to vision-capable models. The transcript already holds AttachmentRef { kind: Image, mime_type, storage_key }; this surfaces the image ones structurally on the model-visible ContextMessage (new ContextImageAttachment { mime_type, storage_key }) instead of only the textual <attachments> pointer. Cheap metadata — no bytes are read here; the turn layer will read+encode them later, and only for a vision-capable model. The text pointer remains the fallback for text-only models. Populated at both context-read paths (window + by-id) in the in-memory and filesystem stores. * feat(reborn): translate image attachments into multimodal parts (image-vision step 2) The model gateway now renders image attachments as real multimodal content for the provider. HostManagedModelMessage carries encoded image parts (HostManagedModelImagePart { mime_type, data_base64 }); convert_messages turns a User message's parts into `ContentPart::ImageUrl` base64 `data:` URLs via `ChatMessage::user_with_parts` (text rides in `content`, images in `content_parts` — the provider adapters prepend the text). Text-only user messages are unchanged. The parts are still empty until step 3 reads+encodes bytes for vision models; this is the gateway-side translation, unit-tested for both the image and text-only paths. * feat(reborn): vision gate + attachment read port (image-vision step 3a) The model gateway now only attaches image parts for a vision-capable model (`is_vision_model(provider_model_id)`); a text-only model keeps just the text (the transcript's `<attachments>` pointer still serves it). And loop_support gains a `LoopAttachmentReadPort` (read bytes by scope + storage_key) plus a `with_attachment_read_port` builder: when wired, `resolve_model_messages` reads each model-visible message's image attachments through the port and base64-encodes them into `HostManagedModelImagePart`s. Until the port is injected (step 3b) it stays None, so behavior is unchanged. Read failures are logged and skipped, never failing the turn. Unit tests cover the gate (vision vs non-vision). * feat(reborn): wire the attachment read port end-to-end (image-vision step 3b) Inject the concrete attachment reader so the multimodal path is now live for the WebChat v2 loop. `ProjectScopedAttachmentReader` reads landed bytes back through the project workspace filesystem (bounded, re-scoped through the MountView authority — never a host path) and implements `LoopAttachmentReadPort`. It is threaded factory → gateway struct → model port (mirroring the `skill_context_source` plumbing), and `build_reborn_runtime` constructs it from the local runtime's workspace filesystem and sets it on `DefaultPlannedRuntimeParts`. When no local runtime is composed it stays `None` (images remain the textual pointer). With this, an image attached to a vision-capable model is read, base64-encoded, and sent as a `ContentPart::ImageUrl`. * docs(threads): image multimodal path is implemented (image-vision step 5) The attachment_context doc no longer describes the vision path as a separate future path — it now points at the implemented multimodal flow (model port reads bytes back, gateway sends ContentPart::ImageUrl) and frames the textual pointer as the text-only-model fallback. * test(4644): cover image-vision producer read path + fix reader error taxonomy Add producer-side integration coverage for the image-vision feature, which previously had only consumer-side unit tests (convert_messages): - thread_loop_support_contract: drive the model port with a landed image attachment + a stub LoopAttachmentReadPort, asserting the resolved HostManagedModelMessage carries the base64-encoded bytes as an image part (test-through-the-caller: the read port gates a side effect with the model port wrapper between). - attachment_landing: round-trip (land -> read back), not-found, and oversized reader tests. The oversized test surfaced a bug: read_bytes_bounded returns Ok(None) only for oversized files (a missing file is Err(NotFound)), so the reader mislabeled oversized attachments as NotFound and missing ones as a generic Backend error. Fix the mapping: Ok(None) -> Backend("exceeds limit"), Err(NotFound) -> NotFound, Err(PermissionDenied) -> Forbidden. * test(4644): set attachment_read_port in reborn parity harness The image-vision field added to DefaultPlannedRuntimeParts was not threaded into the root reborn parity test harness, breaking compilation of every reborn_*_parity integration target (E0063) and the --all-targets clippy/test jobs. Set it to None (these parity tests don't exercise the image path). * feat(openai-compat): vision support for inline images (nearai#4644) Inline base64 image_url parts on /v1/chat/completions now reach a vision-capable model instead of being flattened to [image omitted]. Native non-adapter path (the product-adapter envelope is contractually bytes-free, so inline image bytes cannot ride it): - product_workflow: DefaultInboundTurnService gains attachment landing, and DefaultProductWorkflow exposes submit_inbound_with_attachments(envelope, Vec<InboundAttachment>) — decoded bytes are a direct arg (never serialized into the envelope), threaded to accept_prepared_user_message which lands them via the existing ProjectScopedAttachmentLander and accepts with MessageContent::with_attachments. Shares one pipeline with the bytes-free submit_inbound (no duplicate dispatch). - openai_compat: content_parts decodes image_url 'data:' URLs (base64, MIME allowlist + magic-byte validation, 10 MiB/image cap); carried images are omitted from transcript text (the landed attachment renders its own pointer downstream), while remote/malformed/oversized/mismatched images keep the [image omitted] marker. Defines the OpenAiCompatInboundAttachmentSubmit port (the route crate must not depend on ironclaw_product_workflow — enforced by reborn_dependency_boundaries); the chat workflow calls it when images are present and the door is wired, else falls back to the bytes-free submit. Chat body cap raised to 14 MiB. - composition: wires ProjectScopedAttachmentLander into the inbound turn service and bridges the route-crate port to DefaultProductWorkflow via OpenAiCompatAttachmentSubmitAdapter. Reuses the nearai#4871 downstream model path unchanged (storage_key -> read port -> ContentPart::ImageUrl, vision-gated). product_adapters envelope stays bytes-free. Tests: content_parts decode/validation; product_workflow caller-level landing (lands inline bytes before acceptance; missing lander fails closed); descriptors contract updated for the 14 MiB chat body cap; architecture dependency boundaries hold. * refactor(4644): gate image read via gateway capability + harden attachment paths Code-quality follow-ups to the inline-image vision feature: - Ask the gateway whether its model accepts images, instead of importing the LLM model catalog into the loop-wiring layer. Adds a defaulted `HostManagedModelGateway::route_accepts_images`; the two LLM-backed gateways override it from the exact model they send to (active model / route snapshot), so the loop's image read-gate matches `convert_messages`'s send-gate. The bridge file (`loop_driver_host`) no longer depends on `ironclaw_llm` and the feature-gated helper is gone. This also fixes a latent bug: the live `serve` runtime wires no model-route resolver, so `resolved_model_route` is `None` — the earlier read-gate keyed on it (`unwrap_or(false)`) and silently disabled vision in production. Keying on the gateway's `active_model_name()` (always present) restores it. - Fail loud on dropped attachments. The InboundTurnService trait default now rejects a turn carrying inline bytes instead of silently dropping them. - Collapse the duplicated 14 MiB chat-body cap to a single MAX_CHAT_BODY_BYTES source of truth in descriptors.rs, used by both the ingress body_limit and the in-workflow body check. Adds caller-level coverage: gateway image-capability seam (vision/text models), the non-vision read short-circuit, and the fail-loud default. Builds clean with and without root-llm-provider; clippy clean. * fix(openai-compat): address review — base64 whitespace + unwired-door image fallback Two PR review findings on the inline-image decode path: - HIGH: when no attachment-submit door is wired, a successfully decoded inline image was dropped from the transcript text (omitted as a "carried" attachment) AND not carried (no door) — the image vanished with no trace. Thread an `enable_attachments` flag (= door wired) into content_value_to_text_and_images; when false, image_url parts fall back to the `[image omitted]` marker like any other unsupported part, so an image is never silently lost. - MEDIUM: standard MIME base64 encoders wrap lines, so a data: URL payload can contain newlines/spaces the strict decoder rejects, dropping a valid image to `[image omitted]`. Strip ASCII whitespace before decoding. Regression tests: marker fallback when attachments disabled, and decode through wrapping whitespace. * fix(4644): green CI + address Copilot/CodeRabbit review - CI (cargo-deny): ignore RUSTSEC-2026-0182 (wasmtime WASIp1 fd_renumber leak), same surface justification as the already-ignored 0149 — the WASM sandbox grants guests no WASIp1 fd capabilities, so it's unreachable. `cargo deny check advisories` → ok locally. - workflow: fail closed when submit_inbound_with_attachments gets inline bytes on a non-UserMessage payload — dispatch_payload only lands them for UserMessage, so other kinds silently dropped user files. Now rejected (400) before binding/ledger. Regression test added at the contract tier. - loop_support: drop the now-unused `base64` dependency (main's nearai#4871 moved base64/data-URL formatting into the gateway; loop_support carries raw bytes). - content_parts: annotate the base64 `.ok()?` with `// silent-ok:` — a malformed inline image from untrusted input is expected and falls back to the bounded `[image omitted]` marker, not a server error.
This was referenced Jun 25, 2026
Merged
This was referenced Jul 2, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Image attachment support for vision-capable models (#4644 follow-on)
Closes the gap documented in #4644: an attached image is now sent to a vision-capable model as real multimodal content, not just a text pointer.
Key finding
ironclaw_llmalready had the core image primitives (ContentPart::ImageUrlaccepts base64data:URLs,ChatMessage::user_with_parts,vision_models::is_vision_model); the original gap was the Reborn turn path flattening attachments to text. Update: follow-on commits on this branch do changeironclaw_llm— a sharedImageUrl::decode_data_urlhelper plus image forwarding in the provider adapters that previously dropped it (Anthropic OAuth, Gemini OAuth, Bedrock), and thevision_modelspatterns were corrected for current tier-first Claude ids. The model port now carries raw image bytes and the gateway does the base64/data:encoding only for vision models.What ships (WebChat v2 loop — functional end-to-end)
ContextMessagecarries image refs (ContextImageAttachment { mime_type, storage_key }), populated at both context-read paths in both stores.model_gateway::convert_messagesrenders image parts asContentPart::ImageUrlbase64data:URLs viauser_with_parts(text incontent, images incontent_parts).loop_supportLoopAttachmentReadPort+resolve_model_messagesreads each model-visible message's image attachments through the port and base64-encodes them.convert_messages(is_vision_model(provider_model_id)): a text-only model gets no image parts (keeps the<attachments>text pointer); read failures are logged + skipped, never failing the turn.ProjectScopedAttachmentReaderreads landed bytes back through the project workspace filesystem (bounded, re-scoped via theMountViewauthority — never a host path), wired factory → gateway → model port, constructed inbuild_reborn_runtimefrom the local runtime's workspace filesystem.attachment_contextdoc updated to reflect the implemented path.Tests
convert_messagesconsumer unit tests: vision emits parts, text-only no parts, non-vision drops parts.thread_loop_support_contract): drives the model port with a landed image attachment + a stubLoopAttachmentReadPort, asserting the resolvedHostManagedModelMessagecarries the base64-encoded bytes as an image part. Closes the read→encode loop ("test through the caller").ProjectScopedAttachmentReaderunit tests: round-trip (land → read back), not-found, oversized.read_bytes_boundedreturnsOk(None)only for oversized files (a missing file isErr(NotFound)), so the reader mislabeled oversized →NotFoundand missing → genericBackend. Fixed:Ok(None)→Backend("exceeds limit"),Err(NotFound)→NotFound,Err(PermissionDenied)→Forbidden.attachment_read_portfield (compile fix for the root integration targets).Follow-ups (not in this PR)
#4680) receives images inline in the client request (not via the landed-attachment/storage_keyflow), so it renders[image omitted]today. Passing them through needs a separate carry (it doesn't land attachments like WebChat), so it's its own change. Scoped in detail in #4644 (comment): land inline base64 at the OpenAI-compat ingress and reuse this PR's downstream path unchanged.Notes
build_reborn_runtime).