diff --git a/enterprise/app/controllers/api/v1/accounts/captain/assistants_controller.rb b/enterprise/app/controllers/api/v1/accounts/captain/assistants_controller.rb index 17ce78fba..ec8e8e653 100644 --- a/enterprise/app/controllers/api/v1/accounts/captain/assistants_controller.rb +++ b/enterprise/app/controllers/api/v1/accounts/captain/assistants_controller.rb @@ -25,8 +25,8 @@ class Api::V1::Accounts::Captain::AssistantsController < Api::V1::Accounts::Base def playground response = Captain::Llm::AssistantChatService.new(assistant: @assistant).generate_response( - message_content_multimodal(params[:message_content]), - message_history + additional_message: params[:message_content], + message_history: message_history ) render json: response diff --git a/enterprise/app/helpers/captain/chat_helper.rb b/enterprise/app/helpers/captain/chat_helper.rb index fecb7d6e7..baff0a209 100644 --- a/enterprise/app/helpers/captain/chat_helper.rb +++ b/enterprise/app/helpers/captain/chat_helper.rb @@ -18,72 +18,6 @@ module Captain::ChatHelper raise e end - def message_content_multimodal(message) - # If message has text content, start with that - content_parts = [] - - if message.content.present? - content_parts << { - type: 'text', - text: message.content - } - end - - # Add image content if available - if message.attachments.any? - image_attachments = message.attachments.where(file_type: :image) - - image_attachments.each do |attachment| - image_url = get_attachment_url(attachment) - next unless image_url.present? - - content_parts << { - type: 'image_url', - image_url: { - url: image_url - } - } - end - - # Handle audio transcriptions - audio_transcriptions = extract_audio_transcriptions(message.attachments) - if audio_transcriptions.present? - content_parts << { - type: 'text', - text: audio_transcriptions - } - end - - # Handle other attachment types - other_attachments = message.attachments.where.not(file_type: [:image, :audio]) - if other_attachments.any? - content_parts << { - type: 'text', - text: 'User has shared an attachment' - } - end - end - - # Return just text if no special content, otherwise return array for multimodal - if content_parts.length == 1 && content_parts.first[:type] == 'text' - content_parts.first[:text] - elsif content_parts.any? - content_parts - else - 'Message without content' - end - end - - def get_attachment_url(attachment) - if attachment.external_url.present? - attachment.external_url - elsif attachment.file.attached? - # For uploaded files, we need to generate a public URL - # This will work if the file is stored in a public cloud storage - attachment.file.url if attachment.file.respond_to?(:url) - end - end - def extract_audio_transcriptions(attachments) audio_attachments = attachments.where(file_type: :audio) return '' if audio_attachments.blank? diff --git a/enterprise/app/jobs/captain/conversation/response_builder_job.rb b/enterprise/app/jobs/captain/conversation/response_builder_job.rb index 308e5fd68..eea8b934e 100644 --- a/enterprise/app/jobs/captain/conversation/response_builder_job.rb +++ b/enterprise/app/jobs/captain/conversation/response_builder_job.rb @@ -15,7 +15,7 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob generate_and_process_response end rescue StandardError => e - raise e if e.is_a?(ActiveJob::FileNotFoundError) + raise e if e.is_a?(ActiveStorage::FileNotFoundError) handle_error(e) ensure @@ -27,10 +27,8 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob delegate :account, :inbox, to: :@conversation def generate_and_process_response - latest_message = @conversation.messages.incoming.last @response = Captain::Llm::AssistantChatService.new(assistant: @assistant).generate_response( - message_content_multimodal(latest_message), - collect_previous_messages + message_history: collect_previous_messages ) return process_action('handoff') if handoff_requested? @@ -61,6 +59,65 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob message.message_type == 'incoming' ? 'user' : 'system' end + def message_content_multimodal(message) + parts = [] + + parts << text_part(message.content) if message.content.present? + parts.concat(attachment_parts(message.attachments)) if message.attachments.any? + + finalize_content_parts(parts) + end + + def text_part(text) + { type: 'text', text: text } + end + + def attachment_parts(attachments) + [].tap do |parts| + parts.concat(image_parts(attachments.where(file_type: :image))) + + transcription = extract_audio_transcriptions(attachments) + parts << text_part(transcription) if transcription.present? + + parts << text_part('User has shared an attachment') if attachments.where.not(file_type: %i[image audio]).exists? + end + end + + def image_parts(image_attachments) + image_attachments.each_with_object([]) do |attachment, parts| + url = get_attachment_url(attachment) + next if url.blank? + + parts << { + type: 'image_url', + image_url: { url: url } + } + end + end + + def finalize_content_parts(parts) + return 'Message without content' if parts.blank? + return parts.first[:text] if single_text_part?(parts) + + parts + end + + def single_text_part?(parts) + parts.one? && parts.first[:type] == 'text' + end + + def get_attachment_url(attachment) + return attachment.external_url if attachment.external_url.present? + + return unless attachment.file.attached? + + begin + attachment.file_url + rescue ActiveStorage::FileNotFoundError + nil + end + end + def handoff_requested? @response['response'] == 'conversation_handoff' end diff --git a/enterprise/app/services/captain/llm/assistant_chat_service.rb b/enterprise/app/services/captain/llm/assistant_chat_service.rb index 569931d44..ca8fafaa0 100644 --- a/enterprise/app/services/captain/llm/assistant_chat_service.rb +++ b/enterprise/app/services/captain/llm/assistant_chat_service.rb @@ -12,9 +12,16 @@ class Captain::Llm::AssistantChatService < Llm::BaseOpenAiService register_tools end - def generate_response(input, previous_messages = [], role = 'user') - @messages += previous_messages - @messages << { role: role, content: input } if input.present? + # additional_message: A single message (String) from the user that should be appended to the chat. + # It can be an empty String or nil when you only want to supply historical messages. + # message_history: An Array of already formatted messages that provide the previous context. + # role: The role for the additional_message (defaults to `user`). + # + # NOTE: Parameters are provided as keyword arguments to improve clarity and avoid relying on + # positional ordering. + def generate_response(additional_message: nil, message_history: [], role: 'user') + @messages += message_history + @messages << { role: role, content: additional_message } if additional_message.present? request_chat_completion end diff --git a/enterprise/lib/chat_gpt.rb b/enterprise/lib/chat_gpt.rb index 44afbd641..ad77159cf 100644 --- a/enterprise/lib/chat_gpt.rb +++ b/enterprise/lib/chat_gpt.rb @@ -8,9 +8,9 @@ class ChatGpt @messages = [system_message(context_sections)] end - def generate_response(input, previous_messages = [], role = 'user') - @messages += previous_messages - @messages << { 'role': role, 'content': input } if input.present? + def generate_response(additional_message: nil, message_history: [], role: 'user') + @messages += message_history + @messages << { 'role': role, 'content': additional_message } if additional_message.present? response = request_gpt JSON.parse(response['choices'][0]['message']['content'].strip) diff --git a/spec/enterprise/controllers/api/v1/accounts/captain/assistants_controller_spec.rb b/spec/enterprise/controllers/api/v1/accounts/captain/assistants_controller_spec.rb index 1f6d83d80..80be6f30f 100644 --- a/spec/enterprise/controllers/api/v1/accounts/captain/assistants_controller_spec.rb +++ b/spec/enterprise/controllers/api/v1/accounts/captain/assistants_controller_spec.rb @@ -211,8 +211,8 @@ RSpec.describe 'Api::V1::Accounts::Captain::Assistants', type: :request do expect(response).to have_http_status(:success) expect(chat_service).to have_received(:generate_response).with( - valid_params[:message_content], - valid_params[:message_history] + additional_message: valid_params[:message_content], + message_history: valid_params[:message_history] ) expect(json_response[:content]).to eq('Assistant response') end @@ -232,8 +232,8 @@ RSpec.describe 'Api::V1::Accounts::Captain::Assistants', type: :request do expect(response).to have_http_status(:success) expect(chat_service).to have_received(:generate_response).with( - params_without_history[:message_content], - [] + additional_message: params_without_history[:message_content], + message_history: [] ) end end diff --git a/spec/enterprise/jobs/captain/conversation/response_builder_job_spec.rb b/spec/enterprise/jobs/captain/conversation/response_builder_job_spec.rb index e2765fbdc..ca8d4a6c0 100644 --- a/spec/enterprise/jobs/captain/conversation/response_builder_job_spec.rb +++ b/spec/enterprise/jobs/captain/conversation/response_builder_job_spec.rb @@ -41,11 +41,14 @@ RSpec.describe Captain::Conversation::ResponseBuilderJob, type: :job do it 'includes image URL directly in the message content for OpenAI vision analysis' do # Expect the generate_response to receive multimodal content with image URL - expect(mock_llm_chat_service).to receive(:generate_response) do |content, _history| - # Content should be an array for multimodal - expect(content).to be_an(Array) - expect(content.any? { |part| part[:type] == 'text' && part[:text] == 'Can you help with this error?' }).to be true - expect(content.any? { |part| part[:type] == 'image_url' && part[:image_url][:url] == 'https://example.com/error.jpg' }).to be true + expect(mock_llm_chat_service).to receive(:generate_response) do |**kwargs| + history = kwargs[:message_history] + last_entry = history.last + expect(last_entry[:content]).to be_an(Array) + expect(last_entry[:content].any? { |part| part[:type] == 'text' && part[:text] == 'Can you help with this error?' }).to be true + expect(last_entry[:content].any? do |part| + part[:type] == 'image_url' && part[:image_url][:url] == 'https://example.com/error.jpg' + end).to be true { 'response' => 'I can see the error in your image. It appears to be a database connection issue.' } end