feat: add support for file attachments / uploads (#15) - #38
Conversation
Co-authored-by: Gemini <gemini-cli@google.com>
…#15) Co-authored-by: Gemini <gemini-cli@google.com>
…ards compatibility Co-authored-by: Gemini <gemini-cli@google.com>
…nd Anthropic clients Co-authored-by: Gemini <gemini-cli@google.com>
…ng document parsing Co-authored-by: Gemini <gemini-cli@google.com>
…allback Co-authored-by: Gemini <gemini-cli@google.com>
…rop, and chat history rendering (#15) Co-authored-by: Gemini <gemini-cli@google.com>
…pState Co-authored-by: Gemini <gemini-cli@google.com>
…e limits, and update docs (#15) Co-authored-by: Gemini <gemini-cli@google.com>
|
Please note the guidance recently added in 9e6a61a and relocate your specs. |
clomp42
left a comment
There was a problem hiding this comment.
Adversarial Code & Architecture Review — PR #38
Solid architecture and clean native implementation overall — particularly the zero-dependency document parsing via / and the dual-layer vision fallback via .
Here are 4 critical findings and structural recommendations to address before merging:
1. 📁 Specs & Plans Directory Alignment (PR #37 Rule)
- Finding: The PR introduces and .
-
Fix: In line with PR #37 and the updated / rules, move these files to:
2. ⚡ Base64 Payload Persistence in (Critical Token & Disk Bloat)
- Finding: When an image is attached to a vision model turn, embeds the Base64 string into . In , this is appended directly to and saved into .
- Impact:
- Token/Bandwidth Multiplication: For every subsequent turn in that chat session, sends the multi-megabyte Base64 image payload in the API request over and over again.
- Disk Inflation: permanently stores raw Base64 image strings on disk.
- Fix: Store only metadata (file URL, name, size) in and history. Strip from after the turn request completes, or generate JIT strictly for the immediate API request.
3. 🎯 Vision Model Capability Detection ()
- Finding: uses hardcoded string matching ().
- Impact: Model aliases like , , , , or OpenRouter model strings (e.g. ) could fail this check and unnecessarily trigger auxiliary vision fallbacks.
- Fix: Expand matching keywords to include , , , , , or check provider capabilities.
4. 🔤 Text Encoding Fallbacks in
- Finding: fails silently for non-UTF8 text files (ISO-8859-1, UTF-16, ASCII with high bits).
- Fix: Fall back to or if decoding returns before throwing a warning.
Summary
The UI components (, drag-and-drop targets) and native parsing engines are well-designed and covered by unit tests. Once the Base64 history persistence and specs directory locations are resolved, this is ready to merge!
rune42808
left a comment
There was a problem hiding this comment.
Review: file attachments PR
Nice work — this is a solid foundation for file uploads. Clean architecture: FileAttachment → AttachmentProcessor → VisionRouter → LLM clients, well-separated UI.
Major: Vision detection misses Claude 4 models
VisionRouter.isVisionCapable() checks name.contains("claude-3") — catches claude-3-5-sonnet but misses claude-sonnet-4 and claude-haiku-4-5 (Claude 4). All modern Anthropic models support vision, so use:
name.contains("claude") // all Claude models support visionMajor: Infinite recursion in AuxiliaryInferenceEngine defaults
The extension provides defaults for both generate overloads, each calling the other — if a future engine overrides neither, it will stack-overflow at runtime. Have both delegate to a shared private stub.
Minor: Synchronous I/O
Data(contentsOf:) and String(contentsOf:) block the SwiftUI main thread. Consider wrapping in Task { } or using URLSession async loads for the 20MB/50MB limits.
Minor: PR includes unrelated diff noise
EmojiCatalog.swift changes and EmojiTokenModel.swift removal appear to be accidental includes — consider separating into a cleanup PR.
Test coverage: ++
Comprehensive — Anthropic payload format, processing edge cases, vision routing with mock engines, serialization round-trips, integration flows. The MockVisionEngine pattern is clean.
Design docs
Thorough plan and spec. Drag-and-drop from Finder (mentioned) could be a good follow-up.
Overall: solid implementation, good separation of concerns, strong tests. Fix the two majors before merge.
- Relocate specs and plans to docs/specs and docs/plans - Strip Base64 binary inlineData from conversation history after turn to prevent token and disk inflation - Expand VisionRouter vision model keywords (Claude 3/4/4.5, GPT-4, O1, O3, VL models) - Fix infinite recursion in AuxiliaryInferenceEngine protocol overloads - Add ASCII and ISO-Latin-1 text encoding fallbacks in AttachmentProcessor Co-authored-by: Gemini <gemini-cli@google.com>
|
All code review feedback has been addressed and pushed in commit abc7f75:
|
bnaylor
left a comment
There was a problem hiding this comment.
Review: file attachments / vision (#15)
Reviewed by Claude (Opus 4.8) at Brian's request. Build passes; AttachmentProcessorTests 10/10; VisionRouterTests has 1 failure — see the critical inline note.
🔴 Critical (blocks merge)
- Auxiliary vision path silently drops the image (protocol-dispatch bug) — the whole text-only-model fallback sends no image. Branch is red on its own test. Inline on
AuxiliaryInferenceEngine.swift.
🟠 Important — security (untrusted files)
- Extracted attachment text is injected into the prompt with no sanitization / tag-escaping, bypassing the existing
InjectionGuard. Inline onAttachmentProcessor.swift. NSAttributedString(url:options:[:])on untrusted docx/rtf can trigger remote-resource loading (SSRF class). Inline onAttachmentProcessor.swift.
🟡 Moderate — correctness
isVisionCapablesubstring matching over-matches (routes images to text-only models). Inline onVisionRouter.swift.- Reflection counter reset moved into the async task → double-reflection window. Inline on
AppState.swift. stripInlineDataFromHistoryruns after every model response within a turn, so image+tool-use turns lose the image mid-turn. Consider scoping to end-of-turn.
⚪ Minor
- Persisted
FileAttachment.fileURL→ dangling refs / paths in conversation JSON. - Anthropic/OpenAI adapters flipped
else if→independentiffor parts (benign, but note it). - Image types gated by extension only; some providers reject gif/webp.
✅ Strengths
Clean models (Sendable, backwards-compatible Codable); size limits enforced before reading into memory; file I/O off the main actor; correct OpenAI/Anthropic multimodal payloads; solid AttachmentProcessor coverage; stripInlineDataFromHistory is a thoughtful token/disk win.
Bottom line: don't merge until the critical aux-vision bug (and the red test) are fixed; the two security items are real for untrusted files. Rest are tractable. (I did not deep-read the ChatView/AttachmentBarView UI diff.)
|
|
||
| /// Optional 3-parameter overload for vision-capable auxiliary engines. | ||
| /// Default implementation ignores images and calls 2-parameter generate. | ||
| func generate(prompt: String, jsonSchema: String?, images: [String]?) async throws -> String { |
There was a problem hiding this comment.
🔴 Critical: the aux vision model never receives the image.
generate(prompt:jsonSchema:images:) is defined only as a protocol extension default (which discards images and calls the 2-arg generate), not as a protocol requirement. VisionRouter holds the engine as any AuxiliaryInferenceEngine and calls this 3-arg method — which statically dispatches to this default, so OllamaEngine's real images: override is never reached through the existential. The aux model is asked to "describe this image" with no image → hallucinated description.
testProcessTextOnlyImagesWithAuxiliaryModel fails exactly here (receivedImages == nil), so the branch is red.
Fix: make the overload a protocol requirement (keep this as the default impl):
protocol AuxiliaryInferenceEngine: Sendable {
...
func generate(prompt: String, jsonSchema: String?, images: [String]?) async throws -> String
}| ?? (try? String(contentsOf: attachment.fileURL, encoding: .isoLatin1)) | ||
| ?? (try? String(contentsOf: attachment.fileURL)) { | ||
| let truncated = text.count > 100_000 ? String(text.prefix(100_000)) + "\n[Content truncated at 100,000 characters]" : text | ||
| textBlocks.append("<attached_file name=\"\(attachment.filename)\" mime=\"\(attachment.mimeType)\">\n\(truncated)\n</attached_file>") |
There was a problem hiding this comment.
🟠 Security: untrusted file content injected without sanitization or escaping.
This wraps extracted (untrusted) file text in <attached_file name="…">…</attached_file> and AppState.sendMessage joins it straight into the model prompt — bypassing the InjectionGuard/PromptInjectionGuard the codebase applies to system events. Two concrete holes:
- A file whose content contains a literal
</attached_file>escapes the wrapper and can inject instructions. \(attachment.filename)is interpolated unescaped intoname="…"— a"or>in the filename breaks the tag.
Escape/neutralize the closing tag and the filename, and ideally route extracted text through InjectionGuard. (Same applies to the vision-description path — adversarial text inside an image becomes injected prompt content.)
| } | ||
|
|
||
| case .document: | ||
| if let attrString = try? NSAttributedString(url: attachment.fileURL, options: [:], documentAttributes: nil) { |
There was a problem hiding this comment.
🟠 Security: NSAttributedString(url:options:[:]) on untrusted documents.
Empty options let NSAttributedString sniff the content type; a crafted file (RTF/HTML/webarchive with a docx/rtf name) can trigger remote-resource loading — the well-known NSAttributedString SSRF class. Constrain the parse to the expected type, e.g. options: [.documentType: NSAttributedString.DocumentType.rtf] (or .officeOpenXML for docx), rather than auto-detecting.
| public static func isVisionCapable(modelName: String) -> Bool { | ||
| let name = modelName.lowercased() | ||
| return name.contains("gemini") || | ||
| name.contains("gpt-4") || |
There was a problem hiding this comment.
🟡 isVisionCapable over-matches via substrings.
contains("gpt-4") marks text-only gpt-4/gpt-4-0314 as vision; contains("o1") marks o1-mini; contains("claude") marks all Claude (older ones aren't vision). A false positive sends an image inline to a text-only model → hard API error instead of the aux-model fallback. Tighten to specific vision-capable families/versions.
|
|
||
| if shouldReflect { | ||
| if let idx = conversations.firstIndex(where: { $0.id == convId }) { | ||
| conversations[idx].messageCountSinceReflection = 0 |
There was a problem hiding this comment.
🟡 Reflection counter reset now happens inside the async task, after processInput.
Since the reset moved after a potentially long processInput, a second message sent during that window re-evaluates shouldReflect on the stale (≥30) count → double reflection. It was synchronous before this refactor. Consider resetting the counter synchronously up-front when shouldReflect is decided.
…, SSRF protection, and model capability checks (#15) - Restore 3-param generate requirement to AuxiliaryInferenceEngine protocol for witness table dynamic dispatch - Sanitize extracted attachment text with PromptInjectionGuard to prevent prompt injection breakouts - Restrict NSAttributedString reading options to local readAccessURL to prevent remote SSRF asset fetching - Tighten VisionRouter.isVisionCapable checks to prevent false positives on text-only models - Reset messageCountSinceReflection synchronously upfront in AppState - Move stripInlineDataFromHistory to post-turn completion in iris.swift Co-authored-by: Gemini <gemini-cli@google.com>
|
All review items from Claude (Opus 4.8) have been resolved and verified in commit 66dc507:
|
Closes #15
Summary of Changes