Pass tool result images to OpenAI and Gemini providers#1864
Merged
Conversation
Tool results with image contentBlocks (e.g. browser_screenshot) were silently dropped by the OpenAI and Gemini providers because their APIs don't support images in tool/function-response messages. Fix by injecting images as user message content (OpenAI) or sibling inline data parts (Gemini), which both APIs support. Co-Authored-By: Claude <noreply@anthropic.com>
siddseethepalli
deleted the
sidd/fix-tool-result-images-for-providers
branch
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
ashleeradka
added a commit
that referenced
this pull request
Feb 14, 2026
Resolved modify/delete conflict: IPCMessages.swift was moved to shared/ in this branch, while main had no modifications to port. Changes from main: - Tool result images for OpenAI/Gemini providers (#1864) - Browser screenshot in tool call results (#1863) - Qdrant vector DB clear (#1862) - Debug prompt logger truncation removal (#1861) - macOS release stapler resilience (#1860) No conflicts with shared library changes.
ashleeradka
added a commit
that referenced
this pull request
Feb 14, 2026
The imageData field was added to ToolResultMessage in main (commit ea7c3d7) for browser screenshot support (PR #1864) but was inadvertently dropped during the migration to the shared library. This field enables the daemon to send base64-encoded image data from tool contentBlocks. While not currently consumed by the macOS client, adding it ensures the shared library's protocol definition matches the daemon's implementation. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ashleeradka
added a commit
that referenced
this pull request
Feb 14, 2026
…1821) * Create VellumAssistantShared library target for code reuse - Add multi-platform library target to Package.swift (macOS .v14, iOS .v17) - Move IPC layer (DaemonClient, IPCMessages) to shared library - Add platform-specific connection logic to DaemonClient: - macOS: Unix domain socket at ~/.vellum/vellum.sock - iOS: TCP endpoint (configured via UserDefaults) - Make all IPC types public with necessary initializers - Update macOS app to import VellumAssistantShared in all files using IPC - Add public initializers for Decodable messages used in tests Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Restructure directory layout for multi-platform support Move to flat structure at clients/ level: - Move Package.swift from clients/macos/ to clients/ - Move vellum-assistant-shared/ to clients/shared/ - Add .gitignore at clients/ level to exclude .build/ - Update Package.swift paths to reference shared/ and macos/ This prepares for iOS app in clients/ios/ without nesting everything under an Apple-specific directory. Final structure: clients/ Package.swift (Swift package for all Apple platforms) .gitignore (ignore .build, .swiftpm, etc) shared/ (VellumAssistantShared target) macos/ (macOS app) ios/ (future iOS app) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Address PR feedback: fix Resources directory and port overflow Fixes based on Devin and Codex review comments: 1. Create shared/Resources directory with .gitkeep - Package.swift declares resources: [.process("Resources")] - Git doesn't track empty directories, causing fresh clone failures - Added .gitkeep to ensure directory is tracked 2. Guard iOS daemon port conversion from UserDefaults overflow - Previous: Direct cast Int -> UInt16 can crash if value >65535 or negative - Fixed: Use UInt16(clamping:) with validation (0-65535 range) - Falls back to 8765 for invalid values - Prevents runtime crash from malformed persisted settings Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Add missing public initializers and properties - Add public init to TaskSubmitMessage - Make SessionItem and HistoryMessageItem properties public - Make HistoryToolCallItem properties public Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Fix access control and iOS compatibility issues in shared IPC layer Address critical compilation issues and improve iOS platform handling: 🔴 Critical fixes: - Make SkillSummaryItem typealias public for cross-module access - Make UpdateInfo properties (name, installedVersion, latestVersion) public 🟡 Medium severity fixes: - Add public initializers to all Encodable IPC message structs: • PingMessage, SkillsEnableMessage • ConfirmationResponseMessage, SecretResponseMessage • AddTrustRuleMessage, TrustRulesListMessage • RemoveTrustRuleMessage, UpdateTrustRuleMessage - Add explicit error logging on iOS for unsupported signing operations (signBundlePayload, getSigningIdentity) instead of silently dropping These changes ensure the shared library has consistent public API surface for both macOS and iOS targets, and makes iOS behavior more transparent. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Complete public API consistency and add platform safety guards Address remaining low-severity issues for production-ready shared library: 🔧 API Completeness (Issue #1): - Add public initializers to 17 remaining Encodable structs: • Skills: SkillsDisableMessage, SkillsConfigureMessage, SkillsInstallMessage, SkillsUninstallMessage, SkillsUpdateMessage, SkillsCheckUpdatesMessage, SkillsSearchMessage, SkillsInspectMessage • Apps: AppsListRequestMessage, SharedAppsListRequestMessage, SharedAppDeleteRequestMessage, BundleAppRequestMessage, OpenBundleMessage • Session: SessionListRequestMessage, HistoryRequestMessage • Signing: SignBundlePayloadResponseMessage, GetSigningIdentityResponseMessage - Ensures consistent public API surface for future iOS code 🛡️ Platform Safety (Issue #4): - Add #else fallback in DaemonClient.connect() with #error directive - Prevents silent compilation failures on unsupported platforms (visionOS, watchOS) - Clear compile-time error: "DaemonClient is only supported on macOS and iOS" 📝 Documentation (Issue #5): - Update DaemonClient doc comment to describe both connection types: • macOS: Unix domain socket at ~/.vellum/vellum.sock • iOS: TCP to configurable hostname:port via UserDefaults - Accurately reflects shared library's cross-platform design ✅ Build: passes in 3.70s ✅ All structs now have consistent public init coverage ✅ Future-proof against unsupported platform builds Note: iOS signing request handling (Issue #3) remains logged-only since response messages lack error fields. Proper fix requires daemon-side changes to avoid sending these requests to iOS clients. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Fix SendError visibility and clarify VellumAssistantLib platform restriction Issue #5 (Low severity): - Make SendError enum public for cross-module error handling - Make errorDescription public for LocalizedError protocol conformance - Enables consumers to pattern-match on specific error cases (e.g., .notConnected) Issue #4 (Low severity - documentation): - Add comment clarifying VellumAssistantLib is macOS-only - Links macOS-specific frameworks (AppKit, ScreenCaptureKit, etc.) - iOS apps should depend only on VellumAssistantShared Note: Issue #1 (build script) is NOT a problem - SPM automatically searches parent directories for Package.swift. Build script works correctly as-is. ✅ Build: 4.65s Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Fix platform coverage consistency and remove fragile Resources workaround Issue #1 (Consistency): - Add #else clause to signing operation switch cases in handleServerMessage - Ensures all platforms have explicit handling (logs error on unsupported platforms) - Complements the #error directive in connect() for defense in depth Issue #3 (Type inference fragility): - Replace .init shorthand with fully qualified ConfirmationAllowlistOption type - Consistent with other preview blocks in ToolConfirmationBubble.swift - Prevents breakage if type inference context changes Issue #7 (Build robustness): - Remove empty Resources directory and .process("Resources") declaration - Eliminates fragile .gitkeep workaround (accidental deletion would break builds) - VellumAssistantShared has no resources yet; can add back when needed Issues #2, #4, #5, #6 (Not actionable in this PR): - #2: VellumAssistantLib platform restriction documented via comment (SPM limitation) - #4: @mainactor deinit data race is pre-existing, not introduced by this PR - #5: iOS signing hang acknowledged, needs daemon-side fix - #6: JSON camelCase/snake_case mixed convention is existing protocol, not changing ✅ Build: 5.83s ✅ More robust against platform/type inference edge cases ✅ Removed unnecessary .gitkeep workaround Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Fix @mainactor deinit data race in DaemonClient Issue: Swift concurrency bug (pre-existing, but now fixed) In Swift 5.9+, deinit on a @mainactor class is NOT guaranteed to run on the main actor. When the last reference to DaemonClient is dropped from a background thread, deinit could run on that thread, causing data races when accessing main-actor-isolated properties (shouldReconnect, connection, subscribers, etc.). Solution: Wrap deinit body in MainActor.assumeIsolated { } This ensures: - Safe access to main-actor-isolated state during cleanup - Trap with clear error if deinit somehow runs on wrong thread - Better than silent undefined behavior from data race Why fix in this PR: - Already heavily modifying DaemonClient for shared library - Real correctness issue (undefined behavior, potential crashes) - Simple fix with clear semantics - Makes shared library more robust for both macOS and iOS Reference: SE-0327 (Actors and Deinitializers) ✅ Build: 1.27s ✅ Eliminates undefined behavior from concurrent deinit Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Fix build script clean paths and revise deinit approach Issue #1 (🟡 Medium): Build script clean commands broken - .build directory moved from clients/macos/.build to clients/.build - build.sh clean and release pre-clean still referenced old location - Result: Clean commands did nothing, stale artifacts persisted Fix: Update both clean commands to use $SCRIPT_DIR/../.build - Line 64: Clean command now removes correct directory - Line 83: Release pre-clean now removes correct directory Issue #2 (🔴 High): MainActor.assumeIsolated crash risk in deinit - Previous fix (9231d63) used assumeIsolated which crashes if deinit runs on background thread - While assumeIsolated reveals lifecycle bugs, it's too aggressive for cleanup code that should never crash Fix: Revert to direct access pattern with detailed justification - Task.cancel(), NWConnection.cancel(), Continuation.finish() are all thread-safe - Data races on shouldReconnect and subscribers are benign in deinit (object is being destroyed, no other access possible) - Added comprehensive comment explaining the tradeoff Tradeoff analysis: ✅ No crashes during deallocation ✅ Clean command actually works now ✅ Release builds get proper artifact cleanup⚠️ Technically a data race, but benign for cleanup-only code⚠️ Won't trap on incorrect lifecycle (silent vs. loud failure) Devin review feedback addressed (both comments from 07:40-07:45) ✅ Build: 1.22s Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Add explicit Network framework linking to VellumAssistantShared Issue: VellumAssistantShared imports Network (DaemonClient uses NWConnection) but doesn't explicitly link the Network framework. Currently works because VellumAssistantLib links Network, but future iOS targets depending only on VellumAssistantShared would fail. Fix: Add linkerSettings with Network framework to VellumAssistantShared target While SPM usually auto-links system frameworks on import, explicit linking ensures robust behavior when iOS app target depends only on the shared library (not on VellumAssistantLib which is macOS-only). ✅ Build: 16.92s (full rebuild) ✅ Explicit framework dependencies documented ✅ Future-proof for iOS app target Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Add clients/ directory README for code organization documentation Created comprehensive README.md at clients/ level to document: - Directory structure and target organization - VellumAssistantShared vs VellumAssistantLib vs executable - Platform-specific vs shared code strategy - Build instructions and development guidelines - Migration context from single-platform structure - Known limitations (iOS TCP, signing, localhost) Helps developers and other agents understand: - Why Package.swift is at clients/ level - What code goes in shared/ vs macos/ vs ios/ - How to add new shared/platform-specific code - ~45-50% code reuse strategy Valuable for PR reviewers and future iOS development (PRs 2-13). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Add imageData field to ToolResultMessage for protocol completeness The imageData field was added to ToolResultMessage in main (commit ea7c3d7) for browser screenshot support (PR #1864) but was inadvertently dropped during the migration to the shared library. This field enables the daemon to send base64-encoded image data from tool contentBlocks. While not currently consumed by the macOS client, adding it ensures the shared library's protocol definition matches the daemon's implementation. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Add missing public initializers to IPC message types Added public initializers to 7 Decodable message types that are used in test code. These types need explicit public initializers because Swift's auto-generated memberwise initializers are internal by default: - AssistantThinkingDeltaMessage - CuErrorMessage - CuCompleteMessage - CuActionMessage - GenerationHandoffMessage - MessageDequeuedMessage - GenerationCancelledMessage - ErrorMessage - MessageQueuedMessage Without these, tests that construct messages directly (rather than decoding from JSON) fail with "incorrect argument label" errors. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
Author
|
Addressed in #1901 |
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
1 participant
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
browser_screenshot) were silently dropped by OpenAI and Gemini providers because their APIs don't support images in tool/function-response messagesimage_urlwith base64 data)inlineDataparts alongside the function response in the same user message🤖 Generated with Claude Code