feat(attachments): complete multimodal attachments (upload → LLM, 1:1 + groups) - #588
Merged
Conversation
The transient keyword alone does not stop Jackson's getter-based serialization (no PROPAGATE_TRANSIENT_MARKER configured), so raw attachment base64 payloads were serialized into Mongo conversation documents. Add @JsonIgnore to Attachment.getBase64Data() and prove via serialization tests that the payload never reaches persisted JSON while metadata (mimeType/fileName/storageRef) is preserved. Phase 0 of multimodal-attachments-completion-plan.
The raw attachment_* context map (including its base64 `data` payload)
was persisted into the Mongo conversation document via both the
conversationOutput map and the stored context Data — ~1.33x file size
per turn against the 16MB doc limit, and template-exposed via
{context.attachment_*.data}.
Add AttachmentContextExtractor.scrubInlinePayload(), which returns a
metadata-only copy of an attachment_* context when it carries an inline
payload, and have Conversation.createContextData() build the persisted
copy through it. The live payload still rides ATTACHMENTS memory for the
turn (extracted from the original context map), so LLM forwarding is
unaffected. Mirrors the secret-input scrubbing pattern.
Phase 0 of multimodal-attachments-completion-plan.
…fReaderTool Introduce AttachmentTextExtractor (modules/llm/tools/impl) owning the PDFBox machinery and a plain-text decode path behind a uniform, configurable character cap (eddi.attachments.extraction.max-chars, default 10k). It exposes extractText(bytes, mime[, maxChars]) with PDF + text-like (text/*, JSON, XML, CSV, YAML) dispatch, plus PDF-specific full/page-range/info methods and a canExtractText() capability check. PdfReaderTool now delegates all extraction to this service while keeping its download, SSRF validation and user-facing formatting. This is the shared extractor the Phase 2 forwarder and Phase 4 readAttachment tool will reuse. 22 new unit tests cover the extractor (PdfReaderToolTest remains CI-only — SafeHttpClient opens a loopback selector local JVMs may block). Phase 0 of multimodal-attachments-completion-plan.
New ModelCapabilityService (modules/llm/capability) resolves whether a (provider, model) pair supports vision / native documents / audio / image-by-URL before the forwarder sends content. Resolution precedence: per-task override (Support.ON/OFF/AUTO) > deployment override (eddi.multimodal.<provider>.<cap> then eddi.multimodal.<cap>) > conservative model-aware built-in defaults (plan §5). Unknown provider/model => unsupported => fallback, so we never send content that errors the provider. Injectable via MicroProfile Config; a Function-based constructor keeps it fully unit-testable. 74 tests cover the default matrix across all 11 providers plus override precedence and token parsing. Phase 0 of multimodal-attachments-completion-plan.
… changelog Set quarkus.http.limits.max-body-size=25M so 10-20MB uploads are not rejected with a bare HTTP 413 by Quarkus' 10MB default before the attachment layer sees them. Document eddi.attachments.max-size-bytes and eddi.attachments.extraction.max-chars alongside it. Record Phase 0 in the changelog. Phase 0 of multimodal-attachments-completion-plan (complete).
Collapse the duplicate blob-store abstractions onto a single IAttachmentStore. Previously uploads wrote to IAttachmentStore (GridFsAttachmentStore / PostgresAttachmentStore) while conversation-deletion and GDPR erasure cascaded through a *different* store (IAttachmentStorage → Mongo/PostgresAttachmentStorage), so uploaded blobs were never actually deleted. - Extend IAttachmentStore with getMetadata() (server-validated metadata, no bytes), grantAccess() (trusted-caller-only cross-conversation read grant), and single-item delete() (owner-only). load()/getMetadata() authz is now owner-OR-grant; grants die with the blob. - GridFS: switch the public storageRef to an unguessable random UUID kept in metadata (legacy ObjectId-hex refs still resolve), store grants as a metadata.grants array, enforce per-conversation count + total-byte quotas. - Postgres: add a grants TEXT[] column (additive migration), same quota enforcement; already used UUID refs. - Port the two IAttachmentStorage consumers (RestConversationStore delete cascade, GdprComplianceService erasure) to IAttachmentStore, then delete IAttachmentStorage + MongoAttachmentStorage + PostgresAttachmentStorage and their tests (verified write-dead — only the delete cascades referenced them). New config: eddi.attachments.max-per-conversation (50), eddi.attachments.max-total-bytes-per-conversation (100MB). GridFsAttachmentStoreTest rewritten for UUID refs + grants + quota (26 tests); consumer tests re-typed. Postgres store IT stays CI-only. Phase 1 of multimodal-attachments-completion-plan (part 1/2).
Complete Phase 1 by wiring uploads through to the pipeline and hardening
the REST surface.
- AttachmentContextExtractor: parse {storageRef} (precedence storageRef >
url > data) and add resolveAndGuard(), which resolves each stored ref's
authoritative MIME/size via IAttachmentStore.getMetadata (owner/grant
authorized) before behavior rules run, enforces the per-turn cap, and
records every drop/failure to attachments:errors — never silent. Fixes
the "upload is orphaned" defect (STORED source was never produced).
- Conversation resolves stored metadata at init via new
IPropertiesHandler.getAttachmentStore()/getMaxAttachmentsPerTurn(),
populated by ConversationService (field-injected to avoid touching the
many direct-construction unit tests). New MemoryKeys.ATTACHMENT_ERRORS.
- RestAttachmentUpload: forwardableInline hint on upload (upload cap 20MB >
forward cap 10MB), single-item download endpoint (owner/grant-checked,
Content-Disposition sanitized) and single-item DELETE.
Auth model matches EDDI's anonymous-capable conversations: store-level
owner-or-grant authz + unguessable UUID refs rather than an OIDC role gate
(no other conversation endpoint uses @RolesAllowed).
New config: eddi.attachments.max-per-turn (5), max-forward-bytes (10MB).
+48 unit tests (extractor storageRef/resolveAndGuard, download/delete-one/
forwardableInline). Phase 1 complete.
…ne, caps) Replace the image-only MultimodalMessageEnhancer with AttachmentForwarder — the single place attachments become langchain4j Content on the outgoing user message. Per attachment it resolves bytes from any source (stored blob, URL download via SafeHttpClient, base64 decode) under uniform per-file (10MB) + aggregate (20MB) caps across ALL sources (base64 was previously unguarded), gates on ModelCapabilityService(provider, model), and emits: - image/* -> ImageContent when vision-capable (URL passthrough when the provider fetches URLs, else download-and-inline normalization); else a note - application/pdf -> hybrid: native PdfFileContent when documents supported, else PDFBox text extraction inlined as TextContent - text/*, JSON, XML, CSV, YAML -> decoded + inlined (no capability required) - audio/* -> AudioContent when supported, else a note - else -> metadata note pointing at the readAttachment tool Extracted text -> attachments:extracts (for history stitching); every drop/skip/gate -> attachments:errors AND a relayable note, never silent. LlmTask calls the forwarder with the resolved (provider, model), field-injected + null-guarded so the six direct-construction LlmTask tests are untouched. MultimodalMessageEnhancer + its tests deleted; 18 forwarder tests cover the full branch matrix. Phase 2 (forwarder core) of multimodal-attachments-completion-plan.
Add targeted tests for previously-uncovered branches so every delivered attachment class clears the >90% instruction / >80% branch bar: - AttachmentForwarder: aggregate-cap skip, download non-200, download exception, empty-text note, invalid-base64 note (85->93% instr, 88% branch) - RestAttachmentUpload: list/delete-all/download 500 error paths (84->93%) - GridFsAttachmentStore: null-owner allow, getMetadata grant + null-metadata defaults (79->86% branch) - ModelCapabilityService: full vision-model / text-only substring matrices (77->95% branch) PostgresAttachmentStore mirrors GridFs and is covered by its CI-only Testcontainers IT.
…itching
Complete the Phase 2 tail.
Per-task config: LlmConfiguration.Task gains optional multimodal
{vision|documents|audio: auto|on|off} + reattachTurns (default 0). Old JSON
deserializes cleanly. AttachmentForwarder.forward gains a Support-parameterized
overload; LlmTask parses the task block and applies overrides (per-task >
deployment > default).
History stitching: ConversationLogGenerator.generate gains an opt-in
stitchAttachmentExtracts flag — only the LLM-facing ConversationHistoryBuilder
paths (normal + skipSteps windowing) pass true, so the visible transcript stays
clean. Each past turn's attachments:extracts is appended to its rebuilt user
message. Verified: outputs align 1:1 with getAllSteps(), and non-public step
data survives snapshot persist/reload — so a turn-2 follow-up sees turn-1's
PDF/text extracts. reattachTurns is schema-ready; extracts + the readAttachment
tool are the continuity mechanisms.
Tests: forwarder override on/off, Task config fields, 3 stitching tests;
existing history/log tests unchanged (stitching is inert without extract data).
ReadAttachmentTool (@Vetoed) gives the LLM on-demand access to the conversation's attachments: listAttachments() and readAttachment(nameOrRef, page) — 1-based PDF page or 0 for the whole doc, else a no-extractable-text note. Conversation id is implicit (constructor-injected), so the LLM never supplies userId/conversationId and can only reach its own or granted attachments, enforced by IAttachmentStore. AgentOrchestrator gains setAttachmentServices(store, extractor), wired by LlmTask in a new @PostConstruct after CDI injection (long constructor + its six direct-construction tests untouched). Auto-added in the no-whitelist branch when the turn has attachments, and under whitelist key "readattachment"; skipped when services are unset or no attachments. Forwarder fallback notes already reference the tool. Tests: ReadAttachmentToolTest (11) + 5 orchestrator auto-add branch tests. Phase 4 of multimodal-attachments-completion-plan.
…Phase 3)
Share discussion attachments with every group member.
- IRestGroupConversation.DiscussRequest gains optional attachments
(AttachmentRef = {mimeType,data,url,fileName}) + a 2-arg compat constructor;
IGroupConversationService.discuss/startAndDiscussAsync gain
attachment-carrying overloads.
- GroupConversationService.materializeAttachments stores inline base64 files
in IAttachmentStore bound to the group conversation id (grantable + reapable
with it) and passes url refs through, stashing them on the transient
GroupConversation.attachments.
- On each member's FIRST turn, grantAndInjectAttachments grants the member
conversation read access (the sole grant-minting site — trusted server
code) and injects attachment_* context into its InputData; later phases use
extract-stitching + the readAttachment tool. Nested groups re-grant down the
chain.
- RestGroupConversation converts AttachmentRef -> Attachment and routes
through the attachment overload only when attachments are present, leaving
the no-attachment path (and its tests) untouched.
Transport is JSON inline; a multipart file-part endpoint variant is a thin
follow-up. Tests: 7 service (materialize/grant/inject) + 2 REST routing.
Phase 3 of multimodal-attachments-completion-plan.
Direct branch tests for ConversationLogGenerator.withAttachmentExtracts (null stack/input, out-of-range index, null/empty extracts, present) plus a few reachable generate() compound-condition branches, lifting the class from 71.7% to 83.3% branch. All new/changed attachment classes now clear the >90% instruction / >80% branch gate.
…se 6) - AttachmentForwarder records eddi.attachment.forwarded and eddi.attachment.errors via MeterRegistry (AGENTS.md metrics mandate for the multimodal hot path). - UserDataExport gains an attachments list (AttachmentExportEntry — metadata only, never bytes) + a backward-compatible constructor; GdprComplianceService.exportUserData collects attachment metadata across the user's conversations and records attachmentsExported in the compliance audit event. Deferred follow-ups: nightly reaper (orphan blobs / stale grants), CostTracker multimodal estimates, attachmentsForwarded audit entry, and Phase 5 (multipart say + frontend). The two-step upload->say flow already works end-to-end. Partial Phase 6 of multimodal-attachments-completion-plan.
Cover the backward-compatible (no-attachments) constructor and all nested export-entry accessors. Every new/changed attachment class now clears the >90% instruction / >80% branch gate.
… review 1. Prefix-collision silent data loss (AttachmentForwarder / AgentOrchestrator): getLatestData is a PREFIX scan, and the ATTACHMENTS key "attachments" is a prefix of the attachments:extracts / attachments:errors keys the forwarder persist()s. A second forwarder (or readAttachment auto-add) invocation in the same conversation step reverse-scanned and returned a List<String> extract/ error entry, so readAttachments() found no Attachment and forwarded ZERO attachments with no error note. Reachable with two langchain tasks sharing an action or two langchain workflow steps. Fixed by reading ATTACHMENTS via the exact-match getData(MemoryKey) instead of the prefix getLatestData. 2. Mirror-inverted history stitching (ConversationLogGenerator): withAttachmentExtracts passed the FORWARD conversation-output index into IConversationStepStack.get(), which is REVERSE-ordered (get(0)=newest). In a 3-turn conversation, turn 1's extract surfaced on turn 3's message and turn 1 lost it; only the middle turn aligned. Fixed by converting the forward index to the reverse accessor index (size-1-index). Both escaped the unit tests because they stubbed getLatestData directly and used single-turn (size==1) memories where the reversal is a no-op. Added regression tests: a real ConversationMemory with persisted extracts/errors proving the forwarder still forwards, and a 3-turn stitching test proving extracts land on the correct turn.
…refix collision ContentTypeMatcher (a behavior-rule condition) also reads the short "attachments" key via the prefix-scanning getLatestData and is vulnerable to the same collision with the forwarder-persisted attachments:extracts/errors keys. Switch it to the exact getData read, matching the AttachmentForwarder/AgentOrchestrator fixes. Record the adversarial review outcome in the changelog.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR unifies attachment storage and authorization, adds group-sharing and REST attachment flows, introduces capability-aware multimodal forwarding and extraction tools, persists safer attachment context, exports attachment metadata for GDPR, and documents and tests the completed attachment phases. ChangesMultimodal attachment lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Implements end-to-end multimodal attachment handling across upload/storage, LLM forwarding, multi-turn recall, and group conversations in the EDDI backend, with additional safety measures (non-persistence of inline base64, capability gating, quotas) and expanded tests.
Changes:
- Unifies attachment blob storage behind
IAttachmentStore, adds access grants for group fan-out, and expands REST endpoints for upload/list/download/delete. - Introduces shared
AttachmentTextExtractor, newreadAttachmenttool, attachment extract stitching into LLM history, and model/provider capability resolution viaModelCapabilityService. - Extends group conversation APIs to carry attachments and materializes/grants them to member conversations; adds GDPR export of attachment metadata.
Reviewed changes
Copilot reviewed 60 out of 60 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/ai/labs/eddi/modules/rules/impl/conditions/ContentTypeMatcherTest.java | Updates tests to use exact getData() reads for attachments. |
| src/test/java/ai/labs/eddi/modules/llm/tools/impl/ReadAttachmentToolTest.java | Adds unit tests for the new ReadAttachmentTool. |
| src/test/java/ai/labs/eddi/modules/llm/tools/impl/PdfReaderToolTest.java | Updates PdfReaderTool tests for shared extractor injection. |
| src/test/java/ai/labs/eddi/modules/llm/tools/impl/AttachmentTextExtractorTest.java | Adds comprehensive tests for shared attachment text extraction + truncation. |
| src/test/java/ai/labs/eddi/modules/llm/model/LlmConfigurationTaskTest.java | Adds tests for multimodal override + reattachTurns defaults/behavior. |
| src/test/java/ai/labs/eddi/modules/llm/impl/MultimodalMessageEnhancerTest.java | Removes tests for deleted MultimodalMessageEnhancer. |
| src/test/java/ai/labs/eddi/modules/llm/impl/MultimodalMessageEnhancerExtendedTest.java | Removes extended tests for deleted MultimodalMessageEnhancer. |
| src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorBranchTest.java | Adds branch tests for auto-adding readAttachment tool when attachments present. |
| src/test/java/ai/labs/eddi/engine/memory/rest/RestConversationStoreTest.java | Updates to IAttachmentStore (replacing removed IAttachmentStorage). |
| src/test/java/ai/labs/eddi/engine/memory/rest/RestConversationStoreFilterTest.java | Updates to IAttachmentStore (replacing removed IAttachmentStorage). |
| src/test/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUploadTest.java | Expands tests for forwardable-inline flag, list/delete failures, download & delete-one. |
| src/test/java/ai/labs/eddi/engine/memory/mongo/MongoAttachmentStorageTest.java | Removes tests for deleted legacy MongoAttachmentStorage (old SPI). |
| src/test/java/ai/labs/eddi/engine/memory/model/AttachmentTest.java | Adds tests ensuring base64 payload is never serialized/persisted. |
| src/test/java/ai/labs/eddi/engine/memory/ConversationLogGeneratorTest.java | Adds branch coverage + attachment extract stitching tests. |
| src/test/java/ai/labs/eddi/engine/memory/AttachmentContextExtractorTest.java | Adds tests for stored refs, resolve/guard, and inline payload scrubbing. |
| src/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.java | Adds tests for routing attachment-aware group discuss calls. |
| src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.java | Adds tests for group attachment materialize/grant/inject behavior. |
| src/test/java/ai/labs/eddi/engine/gdpr/UserDataExportTest.java | Adds tests for GDPR export record constructors + attachment metadata entry. |
| src/test/java/ai/labs/eddi/engine/gdpr/GdprComplianceServiceTest.java | Updates GDPR tests to use IAttachmentStore and validate attachment metadata export. |
| src/test/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStorageUnitTest.java | Removes tests for deleted legacy PostgresAttachmentStorage (old SPI). |
| src/test/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStorageTest.java | Removes ITs for deleted legacy PostgresAttachmentStorage (old SPI). |
| src/test/java/ai/labs/eddi/datastore/mongo/MongoAttachmentStorageTest.java | Removes ITs for deleted legacy MongoAttachmentStorage (old SPI). |
| src/main/resources/application.properties | Adds attachment size/limits and extraction cap; raises HTTP max body size. |
| src/main/java/ai/labs/eddi/modules/rules/impl/conditions/ContentTypeMatcher.java | Switches to exact getData() to avoid prefix-collision with attachment keys. |
| src/main/java/ai/labs/eddi/modules/llm/tools/impl/ReadAttachmentTool.java | Adds new built-in tool for listing/reading attachment text (incl. PDF pages). |
| src/main/java/ai/labs/eddi/modules/llm/tools/impl/PdfReaderTool.java | Refactors to delegate PDFBox extraction to shared AttachmentTextExtractor. |
| src/main/java/ai/labs/eddi/modules/llm/tools/impl/AttachmentTextExtractor.java | Adds shared PDF/text extraction service with configurable truncation. |
| src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java | Adds multimodal override block and reattachTurns setting. |
| src/main/java/ai/labs/eddi/modules/llm/impl/MultimodalMessageEnhancer.java | Removes legacy image-only enhancer in favor of unified forwarder. |
| src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java | Integrates attachment forwarding with capability gating + per-task overrides; wires tool services post-construct. |
| src/main/java/ai/labs/eddi/modules/llm/impl/ConversationHistoryBuilder.java | Enables attachment extract stitching when building LLM-facing history. |
| src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java | Wires attachment services + auto-adds readAttachment tool when attachments exist. |
| src/main/java/ai/labs/eddi/modules/llm/capability/ModelCapabilityService.java | Adds model/provider multimodal capability resolver with override precedence. |
| src/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.java | Resolves stored attachment metadata, enforces per-turn caps, persists attachment errors, and scrubs inline payloads from persisted context. |
| src/main/java/ai/labs/eddi/engine/memory/rest/RestConversationStore.java | Updates attachment cleanup wiring to use IAttachmentStore. |
| src/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.java | Adds forwardable-inline response flag and new download/delete-one endpoints; sanitizes Content-Disposition filename. |
| src/main/java/ai/labs/eddi/engine/memory/mongo/MongoAttachmentStorage.java | Removes legacy GridFS storage SPI implementation. |
| src/main/java/ai/labs/eddi/engine/memory/model/Attachment.java | Marks base64 getter @JsonIgnore to prevent persistence of inline payload. |
| src/main/java/ai/labs/eddi/engine/memory/MemoryKeys.java | Adds memory keys for attachment errors and text extracts. |
| src/main/java/ai/labs/eddi/engine/memory/IPropertiesHandler.java | Exposes attachment store + per-turn cap for attachment resolution at init. |
| src/main/java/ai/labs/eddi/engine/memory/IAttachmentStorage.java | Removes legacy attachment storage SPI (folded into IAttachmentStore). |
| src/main/java/ai/labs/eddi/engine/memory/ConversationLogGenerator.java | Adds optional attachment extract stitching aligned to step/output indexing. |
| src/main/java/ai/labs/eddi/engine/memory/AttachmentContextExtractor.java | Adds stored-ref parsing, resolve/guard logic, and inline payload scrubbing for persistence. |
| src/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.java | Extends group discuss endpoints to accept attachments and call attachment-aware service overloads. |
| src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java | Materializes group attachments into store, grants access to members, injects attachment_* contexts, and propagates to nested groups. |
| src/main/java/ai/labs/eddi/engine/internal/ConversationService.java | Provides attachment store + per-turn cap to conversation init via IPropertiesHandler. |
| src/main/java/ai/labs/eddi/engine/gdpr/UserDataExport.java | Adds attachment metadata to GDPR export record (with backward-compatible constructor). |
| src/main/java/ai/labs/eddi/engine/gdpr/GdprComplianceService.java | Includes attachment metadata in GDPR export and updates attachment deletion wiring. |
| src/main/java/ai/labs/eddi/engine/attachments/IAttachmentStore.java | Expands store contract: metadata reads, grants, single-item delete, clarified ownership/authz model. |
| src/main/java/ai/labs/eddi/engine/api/IRestGroupConversation.java | Adds attachments to DiscussRequest and defines AttachmentRef. |
| src/main/java/ai/labs/eddi/engine/api/IGroupConversationService.java | Adds attachment-aware overloads (default methods) for discuss/start-and-discuss. |
| src/main/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStore.java | Adds grants, per-conversation quotas, metadata reads, single delete, and authorization for grants. |
| src/main/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStorage.java | Removes legacy PostgreSQL attachment storage SPI implementation. |
| src/main/java/ai/labs/eddi/datastore/mongo/GridFsAttachmentStore.java | Adds UUID-based unguessable refs, grants, quotas, and owner-or-grant authorization; legacy ObjectId refs still resolve. |
| src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java | Adds transient attachments field (not persisted) to carry group discussion attachments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ginccc
force-pushed
the
feat/multimodal-attachments-completion
branch
from
July 3, 2026 08:28
08c7f72 to
1a40952
Compare
- (High) readAttachment couldn't see group-shared blobs: listByConversation is
owner-only, but group attachments are owned by the group conversation and
granted to members. Add IAttachmentStore.listAccessible (owned OR granted) to
both backends (GridFS grants-array match / Postgres = ANY(grants)); the
readAttachment tool lists/resolves through it.
- (Medium) materializeAttachments dropped url attachments when the store is null;
restructure so only the inline-base64 path requires a store.
- (Medium) replace brittle message.contains("denied") with a typed
AttachmentAccessDeniedException (thrown by both backends' authz/delete paths);
REST maps it to 403 and other store errors to 404/500.
- (Note) remove an unused local variable in a GridFS test.
Tests updated/added; 277 green across affected classes.
niedch
approved these changes
Jul 8, 2026
niedch
left a comment
niedch
left a comment
Collaborator
There was a problem hiding this comment.
Great stuff, looks good to me! Just had some nitpicks and maybe an item for the future
Address @niedch PR #588 review: the field-injected IAttachmentStore in ConversationService and GroupConversationService used fully-qualified type references (and, in GroupConversationService, a fully-qualified @jakarta.inject.Inject even though jakarta.inject.Inject was already imported). Add IAttachmentStore imports and use @Inject / simple type names. No behavior change.
Integrate 148 commits from main (HITL framework, Slack approval surfaces, coverage tests) into the multimodal-attachments branch. Resolved 13 files (17 hunks). Key resolution: our branch unified IAttachmentStorage into IAttachmentStore (the old SPI was deleted here). main's side of several conflicts still referenced the now-deleted IAttachmentStorage, but the merged bodies already use IAttachmentStore, so those dead references were dropped. main's deleteByConversation(conversationId) maps 1:1 onto IAttachmentStore, so the GDPR erasure cascade is preserved. Notable combinations (both sides kept): - GdprComplianceService: attachment erasure (IAttachmentStore) + new HITL tool-journal deletion (IHitlToolJournalStore). - ConversationService / AgentOrchestrator / LlmConfiguration: attachment fields + main's HITL fields (hitlResumeCompletedEvent, journalStore, ConversationHistoryBuilder, per-task toolApprovals override). - GroupConversation model: getAttachments/setAttachments + main's HITL pause-state accessors (isPaused now @JsonIgnore). - GroupConversationService: materializeAttachments retained; call updated to main's new executeDiscussion(..., startPhaseIndex) signature. - RestGroupConversation: adopted main's createStreamingListener() helper (a superset of our inline listener, adds HITL/cancel SSE events) while keeping our attachment branching in discussStreaming. - changelog: both entry sets preserved. Verified: clean test-compile (main + test) passes; targeted unit tests green (GdprComplianceServiceTest, GroupConversationServiceTest, RestConversationStore*, AgentOrchestratorBranchTest, AttachmentForwarderTest, LlmTaskTest, ReadAttachmentToolTest, AttachmentContextExtractorTest, RestAttachmentUploadTest, DataStoreProducersBranchTest, AttachmentTextExtractorTest, RestGroupConversationTest).
Add an Imports subsection to AGENTS.md §4.7 (Best Practices & Common Pitfalls): always import types/annotations and reference them by simple name; the only acceptable inline fully-qualified name is disambiguating two same-named classes used in one file. Codifies the recurring PR review comment that prompted the FQN->import cleanup in ConversationService and GroupConversationService.
A critical adversarial re-review of the origin/main merge surfaced a merge-emergent bug: group-shared attachments were silently lost across a HITL pause/resume. GroupConversation.attachments is @JsonIgnore transient (the durable copy lives in the blob store). resumeDiscussion() reloads a fresh GC from the store, so getAttachments() is null; executeDiscussion() re-seeded the sibling transient field dynamicAgentConfig but not attachments. A member speaking for the first time after a resume therefore got neither the blob-store grant nor the attachment_* context. Add rehydrateAttachmentsFromStore(gc), called in executeDiscussion right after the dynamicAgentConfig re-seed, rebuilding the metadata list from IAttachmentStore.listByConversation(gc.getId()) when the in-memory list is empty. Keeps the blob store as the single source of truth (no dangling refs after erasure), no persistence-schema change. Guarded by null/empty rather than startPhaseIndex, because a task-level pause in phase 0 resumes at index 0. URL-only attachments are not blob-backed and are not recovered on resume (documented as a known limitation). Neither merge parent could exhibit this: our branch had group attachments but no resumeDiscussion; origin/main had resumeDiscussion but no group attachments. Adds 4 unit tests (rehydrate_*).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/ai/labs/eddi/modules/rules/impl/conditions/ContentTypeMatcherTest.java (1)
44-51: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale helper Javadoc references
getLatestData. The comment still says it "stubscurrentStep.getLatestData(any())", but the helper now stubsgetData(...). Update the doc to avoid misleading maintainers.📝 Proposed doc fix
/** - * Helper: stubs {`@code` currentStep.getLatestData(any())} to return the given + * Helper: stubs {`@code` currentStep.getData(any())} to return the given * data. Uses {`@code` doReturn().when()} to avoid unchecked generic warnings that * arise with {`@code` when().thenReturn()} on generic methods. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/ai/labs/eddi/modules/rules/impl/conditions/ContentTypeMatcherTest.java` around lines 44 - 51, Update the Javadoc for stubAttachments to describe stubbing currentStep.getData(...) instead of currentStep.getLatestData(...), matching the method invoked by the helper while preserving the existing explanation about doReturn().when().
🧹 Nitpick comments (6)
src/test/java/ai/labs/eddi/datastore/mongo/GridFsAttachmentStoreTest.java (1)
104-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
whenFindIteratesilently breaks for 3+ files.The
hasNextarray is computed (110-113) but never used — the actual mocking is hardcoded to 0/1/2-file branches (114-122). If a future test passes 3+ files,cursor.next()will returnnullbeyond the 2nd call instead of the 3rd file, andhasNext()won't reflect the true count, risking a silent test bug / NPE in code under test.♻️ Proposed generalization using the already-computed array
- Boolean[] hasNext = new Boolean[files.length + 1]; - for (int i = 0; i < files.length; i++) - hasNext[i] = true; - hasNext[files.length] = false; - if (files.length == 0) { - when(cursor.hasNext()).thenReturn(false); - } else if (files.length == 1) { - when(cursor.hasNext()).thenReturn(true, false); - when(cursor.next()).thenReturn(files[0]); - } else { - when(cursor.hasNext()).thenReturn(true, true, false); - when(cursor.next()).thenReturn(files[0], files[1]); - } + Boolean[] hasNext = new Boolean[files.length + 1]; + for (int i = 0; i < files.length; i++) + hasNext[i] = true; + hasNext[files.length] = false; + when(cursor.hasNext()).thenReturn(hasNext[0], Arrays.copyOfRange(hasNext, 1, hasNext.length)); + if (files.length > 0) { + when(cursor.next()).thenReturn(files[0], Arrays.copyOfRange(files, 1, files.length)); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/ai/labs/eddi/datastore/mongo/GridFsAttachmentStoreTest.java` around lines 104 - 123, Update whenFindIterate to use the computed hasNext sequence and all provided files when stubbing cursor.hasNext() and cursor.next(), removing the hardcoded zero/one/two-file branches. Ensure any number of files, including three or more, returns each corresponding file and then reports no additional elements.src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.java (1)
170-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
Contextand use the simple name here
Map<String, ai.labs.eddi.engine.model.Context>appears four times in this test file, and there’s no conflictingContextimport. A top-level import would make the repeated declarations cleaner and match the import-style guideline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.java` at line 170, Import ai.labs.eddi.engine.model.Context at the top of GroupConversationServiceTest and replace all four fully qualified ai.labs.eddi.engine.model.Context references with the simple Context name, preserving the existing generic declarations.Source: Coding guidelines
src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java (1)
693-721: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
MultimodalOverrideis a mutable class rather than a record.As per coding guidelines for
*Configuration.java: "Use Java records for new task configuration POJOs." Note the tension: every sibling nested config type in this file is a mutable Jackson-bound POJO, so converting only this one to a record would be inconsistent and may require@JsonCreatorwiring. Confirm the intended convention for new nested config types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java` around lines 693 - 721, Resolve the configuration POJO convention for MultimodalOverride by following the established pattern in sibling nested types in LlmConfiguration; either convert it to a Java record with the required Jackson binding support or retain the mutable POJO and align the applicable configuration guideline, without leaving this type inconsistent with the chosen convention.Source: Coding guidelines
src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java (1)
84-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the
Attachmentimport here — Replace the inlinedai.labs.eddi.engine.memory.model.Attachmentreferences on the field, getter, and setter with the existing top-level import.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java` around lines 84 - 85, Update the attachments field and its getter/setter in GroupConversation to use the existing Attachment import instead of fully qualified ai.labs.eddi.engine.memory.model.Attachment references, preserving the current types and behavior.Source: Coding guidelines
src/main/java/ai/labs/eddi/modules/llm/impl/AttachmentForwarder.java (1)
79-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
MeterRegistryand pre-initialize the counters in@PostConstruct.Two guideline deviations around the metrics wiring:
io.micrometer.core.instrument.MeterRegistryis referenced by its fully qualified name (field Line 81, constructor Line 88). Use a top-level import and the simple name.- The counters
eddi.attachment.forwarded/eddi.attachment.errorsare resolved on everyforward()call inrecordMetrics(Lines 174, 177). Initialize the reusableCounterinstances once in a@PostConstructmethod and reference the fields.As per coding guidelines: "Use simple names with top-level imports; do not inline fully qualified names except to disambiguate" and "Add Micrometer counters, timers, or gauges to new features and initialize reusable metrics in
@PostConstruct".♻️ Sketch
// top of file import io.micrometer.core.instrument.Counter; import io.micrometer.core.instrument.MeterRegistry; import jakarta.annotation.PostConstruct;- private final io.micrometer.core.instrument.MeterRegistry meterRegistry; + private final MeterRegistry meterRegistry; + private Counter forwardedCounter; + private Counter errorsCounter; @@ - io.micrometer.core.instrument.MeterRegistry meterRegistry, + MeterRegistry meterRegistry,`@PostConstruct` void initMetrics() { if (meterRegistry != null) { forwardedCounter = meterRegistry.counter("eddi.attachment.forwarded"); errorsCounter = meterRegistry.counter("eddi.attachment.errors"); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/llm/impl/AttachmentForwarder.java` around lines 79 - 99, Import MeterRegistry, Counter, and PostConstruct, then replace the fully qualified MeterRegistry references in AttachmentForwarder with the simple name. Add reusable forwarded and error Counter fields and initialize them once in a `@PostConstruct` method using the existing meterRegistry null guard; update recordMetrics to use those fields instead of resolving counters on every forward call.Source: Coding guidelines
src/main/java/ai/labs/eddi/modules/llm/tools/impl/AttachmentTextExtractor.java (1)
110-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract a shared
withDocumenthelper to reduce duplicated PDDocument load/exception-wrap logic.
extractPdfText(byte[], int),extractPdfText(byte[], int, int, int), andextractPdfInfoeach independently openLoader.loadPDF(pdfBytes)and wrap failures inAttachmentExtractionException. A small private helper taking aFunction<PDDocument, T>would remove the repetition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/llm/tools/impl/AttachmentTextExtractor.java` around lines 110 - 176, Optionally introduce a private withDocument helper that loads and closes the PDDocument, applies a Function<PDDocument, T>, and wraps loading or processing failures in AttachmentExtractionException. Refactor extractPdfText(byte[], int), extractPdfText(byte[], int, int, int), and extractPdfInfo to use it while preserving their existing validation, logging, return values, and error messages where applicable.
🤖 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 `@src/main/java/ai/labs/eddi/engine/gdpr/GdprComplianceService.java`:
- Around line 291-306: Update the attachment metadata export block to wrap each
conversation’s attachment lookup and entry construction in its own try/catch,
matching the per-conversation isolation used in the conversations export block.
Keep iterating over all conversation IDs after a failure, log the affected
conversation and pseudonym, and retain the existing resolvability check and
successful attachment collection behavior.
In `@src/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.java`:
- Around line 264-284: Update the warning log in the attachment-resolution block
of Conversation so each “Attachment issue” message includes
conversationMemory.getConversationId() alongside the error details. Keep the
existing warning level and error iteration unchanged.
---
Outside diff comments:
In
`@src/test/java/ai/labs/eddi/modules/rules/impl/conditions/ContentTypeMatcherTest.java`:
- Around line 44-51: Update the Javadoc for stubAttachments to describe stubbing
currentStep.getData(...) instead of currentStep.getLatestData(...), matching the
method invoked by the helper while preserving the existing explanation about
doReturn().when().
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java`:
- Around line 84-85: Update the attachments field and its getter/setter in
GroupConversation to use the existing Attachment import instead of fully
qualified ai.labs.eddi.engine.memory.model.Attachment references, preserving the
current types and behavior.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/AttachmentForwarder.java`:
- Around line 79-99: Import MeterRegistry, Counter, and PostConstruct, then
replace the fully qualified MeterRegistry references in AttachmentForwarder with
the simple name. Add reusable forwarded and error Counter fields and initialize
them once in a `@PostConstruct` method using the existing meterRegistry null
guard; update recordMetrics to use those fields instead of resolving counters on
every forward call.
In `@src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java`:
- Around line 693-721: Resolve the configuration POJO convention for
MultimodalOverride by following the established pattern in sibling nested types
in LlmConfiguration; either convert it to a Java record with the required
Jackson binding support or retain the mutable POJO and align the applicable
configuration guideline, without leaving this type inconsistent with the chosen
convention.
In
`@src/main/java/ai/labs/eddi/modules/llm/tools/impl/AttachmentTextExtractor.java`:
- Around line 110-176: Optionally introduce a private withDocument helper that
loads and closes the PDDocument, applies a Function<PDDocument, T>, and wraps
loading or processing failures in AttachmentExtractionException. Refactor
extractPdfText(byte[], int), extractPdfText(byte[], int, int, int), and
extractPdfInfo to use it while preserving their existing validation, logging,
return values, and error messages where applicable.
In `@src/test/java/ai/labs/eddi/datastore/mongo/GridFsAttachmentStoreTest.java`:
- Around line 104-123: Update whenFindIterate to use the computed hasNext
sequence and all provided files when stubbing cursor.hasNext() and
cursor.next(), removing the hardcoded zero/one/two-file branches. Ensure any
number of files, including three or more, returns each corresponding file and
then reports no additional elements.
In
`@src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.java`:
- Line 170: Import ai.labs.eddi.engine.model.Context at the top of
GroupConversationServiceTest and replace all four fully qualified
ai.labs.eddi.engine.model.Context references with the simple Context name,
preserving the existing generic declarations.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: caf70acf-e8c7-4f13-bfed-979cd3a39c60
📒 Files selected for processing (61)
AGENTS.mddocs/changelog.mdsrc/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.javasrc/main/java/ai/labs/eddi/datastore/mongo/GridFsAttachmentStore.javasrc/main/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStorage.javasrc/main/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStore.javasrc/main/java/ai/labs/eddi/engine/api/IGroupConversationService.javasrc/main/java/ai/labs/eddi/engine/api/IRestGroupConversation.javasrc/main/java/ai/labs/eddi/engine/attachments/IAttachmentStore.javasrc/main/java/ai/labs/eddi/engine/gdpr/GdprComplianceService.javasrc/main/java/ai/labs/eddi/engine/gdpr/UserDataExport.javasrc/main/java/ai/labs/eddi/engine/internal/ConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.javasrc/main/java/ai/labs/eddi/engine/memory/AttachmentContextExtractor.javasrc/main/java/ai/labs/eddi/engine/memory/ConversationLogGenerator.javasrc/main/java/ai/labs/eddi/engine/memory/IAttachmentStorage.javasrc/main/java/ai/labs/eddi/engine/memory/IPropertiesHandler.javasrc/main/java/ai/labs/eddi/engine/memory/MemoryKeys.javasrc/main/java/ai/labs/eddi/engine/memory/model/Attachment.javasrc/main/java/ai/labs/eddi/engine/memory/mongo/MongoAttachmentStorage.javasrc/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.javasrc/main/java/ai/labs/eddi/engine/memory/rest/RestConversationStore.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.javasrc/main/java/ai/labs/eddi/modules/llm/capability/ModelCapabilityService.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AttachmentForwarder.javasrc/main/java/ai/labs/eddi/modules/llm/impl/ConversationHistoryBuilder.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/main/java/ai/labs/eddi/modules/llm/impl/MultimodalMessageEnhancer.javasrc/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.javasrc/main/java/ai/labs/eddi/modules/llm/tools/impl/AttachmentTextExtractor.javasrc/main/java/ai/labs/eddi/modules/llm/tools/impl/PdfReaderTool.javasrc/main/java/ai/labs/eddi/modules/llm/tools/impl/ReadAttachmentTool.javasrc/main/java/ai/labs/eddi/modules/rules/impl/conditions/ContentTypeMatcher.javasrc/main/resources/application.propertiessrc/test/java/ai/labs/eddi/datastore/mongo/GridFsAttachmentStoreTest.javasrc/test/java/ai/labs/eddi/datastore/mongo/MongoAttachmentStorageTest.javasrc/test/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStorageTest.javasrc/test/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStorageUnitTest.javasrc/test/java/ai/labs/eddi/engine/gdpr/GdprComplianceServiceTest.javasrc/test/java/ai/labs/eddi/engine/gdpr/UserDataExportTest.javasrc/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestGroupConversationTest.javasrc/test/java/ai/labs/eddi/engine/memory/AttachmentContextExtractorTest.javasrc/test/java/ai/labs/eddi/engine/memory/ConversationLogGeneratorTest.javasrc/test/java/ai/labs/eddi/engine/memory/model/AttachmentTest.javasrc/test/java/ai/labs/eddi/engine/memory/mongo/MongoAttachmentStorageTest.javasrc/test/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUploadTest.javasrc/test/java/ai/labs/eddi/engine/memory/rest/RestConversationStoreFilterTest.javasrc/test/java/ai/labs/eddi/engine/memory/rest/RestConversationStoreTest.javasrc/test/java/ai/labs/eddi/modules/llm/capability/ModelCapabilityServiceTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorBranchTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AttachmentForwarderTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/MultimodalMessageEnhancerExtendedTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/MultimodalMessageEnhancerTest.javasrc/test/java/ai/labs/eddi/modules/llm/model/LlmConfigurationTaskTest.javasrc/test/java/ai/labs/eddi/modules/llm/tools/impl/AttachmentTextExtractorTest.javasrc/test/java/ai/labs/eddi/modules/llm/tools/impl/PdfReaderToolTest.javasrc/test/java/ai/labs/eddi/modules/llm/tools/impl/ReadAttachmentToolTest.javasrc/test/java/ai/labs/eddi/modules/rules/impl/conditions/ContentTypeMatcherTest.java
💤 Files with no reviewable changes (10)
- src/test/java/ai/labs/eddi/modules/llm/impl/MultimodalMessageEnhancerTest.java
- src/main/java/ai/labs/eddi/engine/memory/IAttachmentStorage.java
- src/test/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStorageTest.java
- src/test/java/ai/labs/eddi/engine/memory/mongo/MongoAttachmentStorageTest.java
- src/test/java/ai/labs/eddi/modules/llm/impl/MultimodalMessageEnhancerExtendedTest.java
- src/main/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStorage.java
- src/test/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStorageUnitTest.java
- src/main/java/ai/labs/eddi/engine/memory/mongo/MongoAttachmentStorage.java
- src/main/java/ai/labs/eddi/modules/llm/impl/MultimodalMessageEnhancer.java
- src/test/java/ai/labs/eddi/datastore/mongo/MongoAttachmentStorageTest.java
Correctness: - Download 404-vs-500 (High): load/getMetadata threw a bare AttachmentStoreException for both a missing blob and an internal store failure, so downloadAttachment mapped SQL/backend errors to 404 at DEBUG. Add a typed AttachmentNotFoundException (symmetric with AttachmentAccessDeniedException); both stores throw it for missing blobs; the endpoint returns 404 for it and 500 (ERROR, ATTACHMENT_STORE_ERROR) for any other store exception. +regression test. - GDPR export isolation (Major): the attachment-metadata export wrapped the whole conversation loop in one try/catch, so one failing listByConversation truncated the export for every remaining conversation. Isolate per conversation, mirroring the conversation-snapshot block. - URL group attachment without mimeType (Medium): toAttachments kept URL refs with null/blank mimeType that AttachmentContextExtractor drops later; skip them up front so the loss is explicit. Observability: - AttachmentForwarder: init reusable Counters once (constructor) instead of resolving per forward(); import MeterRegistry/Counter. - AttachmentTextExtractor: per-extraction PDF logs INFO -> DEBUG. - Conversation: include conversation id in the attachment-issue warning. Style (AGENTS.md import guideline): - LlmTask @jakarta.inject.Inject -> @Inject; GroupConversation imports Attachment; GroupConversationServiceTest imports Context. - GridFsAttachmentStoreTest.whenFindIterate generalized to any file count. Declined: MultimodalOverride stays a mutable Jackson POJO for consistency with its sibling nested config types.
LlmConfiguration.Task.reattachTurns (@SInCE 6.1.0, added on this branch) was dead config: getReattachTurns() is called nowhere in src/main, so setting it did nothing at runtime. Past-turn attachments already reach the model via text-extract stitching (attachments:extracts), not native re-attachment. Remove the field, getter/setter, and its round-trip test. Found by a codebase-wide dead-config audit; the other candidate no-op knobs it surfaced are triaged and tracked as follow-ups rather than mass-deleted (see changelog).
13 tasks
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.
Summary
Implements
planning/multimodal-attachments-completion-plan.mdend-to-end: a user can upload a file (image, PDF, text/docs), have it forwarded appropriately to the LLM in both 1:1 and group conversations, on any configured provider, and recall it in later turns via a dedicated tool.Type of Change
Related Issue
Not tracked by an issue — scoped and delivered directly from
planning/multimodal-attachments-completion-plan.md.Changes Made
@JsonIgnorethe base64 payload so it's never persisted (thetransientkeyword didn't stop Jackson); scrub inline base64 from persistedattachment_*context copies; sharedAttachmentTextExtractor(PDFBox extracted out ofPdfReaderTool);ModelCapabilityService(vision/documents/audio/image-URL, conservative model-aware defaults + per-task/deployment overrides); alignmax-body-sizeabove the attachment cap.IAttachmentStore(deletes/GDPR cascades were hitting a different store than uploads, so blobs were never actually deleted). AddedgetMetadata/grantAccess/single-itemdeletewith owner-or-grant authz, UUID ref hardening, per-conversation quotas, thestorageRefextraction branch (fixes an orphaned-upload bug), and an owner-checked download/delete REST surface.AttachmentForwarder. Replaces the image-onlyMultimodalMessageEnhancer: uniform per-file/aggregate caps across all sources, capability-gated hybrid PDF handling (nativePdfFileContentvs PDFBox text), universal text inline, provider image-URL normalization, extracts/errors persisted (never silent). Plus per-taskmultimodaloverrides and history extract-stitching so later turns retain PDF/text content.DiscussRequestcarries attachments; the service materializes them bound to the group conversation, grants each member conversation at fan-out (the sole grant-minting site), injectsattachment_*on the member's first turn, and re-grants down nested groups.readAttachmenttool. On-demand multi-turn recall (list + read, PDF page-targeted); implicit conversation id (never LLM-supplied); auto-added to the toolset when the turn has attachments. Listing/resolution goes through a new grant-awarelistAccessible()so group members can discover attachments shared with (not owned by) them.AttachmentAccessDeniedExceptionreplaces string-matching on error messages for REST 403-vs-404 handling; group attachment materialization preserves URL-based attachments even when no blob store is configured (only inline base64 needs one).How to Test
./mvnw clean verify -DskipITs— 654 tests pass; JaCoCo enforces the repo's >90% instruction / >80% branch gate on every new/changed class. (DB/HTTP integration tests remain CI-only — local JVMs can't open the loopback selectorSafeHttpClientneeds.)DiscussRequest; confirm each member conversation can see and reason about the shared file (owned by the group conversation, granted to members).readAttachmenttool lists a previously uploaded file by name and reads it back (including page-targeted PDF reads), for both a 1:1 conversation and a group member conversation.Checklist
./mvnw clean verify -DskipITs)Deferred follow-ups
Phase 5 (multipart 1:1
say+ EDDI-Manager / eddi-chat-ui frontend work, which live in separate repos — the two-step upload→say flow already works today) and the rest of Phase 6 (nightly orphan-blob reaper, cost estimates,attachmentsForwardedaudit entry). Group attachment transport is JSON inline; a multipart file-part endpoint variant would be a thin follow-up.Summary by CodeRabbit