Merge branch 'develop' into fix/captain-handoff-activity-message

enterprise/app/jobs/captain/conversation/response_builder_job.rb

@@ -18,7 +18,15 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob
 
     handle_error(e)
   ensure
+    # Process handoff OUTSIDE the transaction so after_commit callbacks work properly
+    # This is because within a transaction, each save operation overrides previous_changes
+    # There's a separate gem to handle this: https://github.com/Shopify/ar_transaction_changes/
+    # But for us, moving the handoff outside the transaction works well enough
+    # This ensures Rails' dirty tracking (previous_changes, saved_change_to_*?) works correctly
+    process_handoff if @handoff_requested
+
     Current.executed_by = nil
+    Current.handoff_requested = nil
   end
 
   private
@@ -36,7 +44,10 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob
                   )
                 end
 
-    return process_action('handoff') if handoff_requested?
+    if handoff_requested? || Current.handoff_requested
+      @handoff_requested = true
+      return
+    end
 
     create_messages
     Rails.logger.info("[CAPTAIN][ResponseBuilderJob] Incrementing response usage for #{account.id}")
@@ -73,13 +84,10 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob
     @response['response'] == 'conversation_handoff'
   end
 
-  def process_action(action)
-    case action
-    when 'handoff'
-      I18n.with_locale(@assistant.account.locale) do
-        create_handoff_message
-        @conversation.bot_handoff!
-      end
+  def process_handoff
+    I18n.with_locale(@assistant.account.locale) do
+      create_handoff_message
+      @conversation.bot_handoff!
     end
   end
 
@@ -114,7 +122,7 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob
 
   def handle_error(error)
     log_error(error)
-    process_action('handoff')
+    @handoff_requested = true
     true
   end
 

enterprise/lib/captain/tools/handoff_tool.rb

@@ -23,19 +23,11 @@ class Captain::Tools::HandoffTool < Captain::Tools::BasePublicTool
 
   private
 
-  def trigger_handoff(conversation, reason)
-    # post the reason as a private note
-    conversation.messages.create!(
-      message_type: :outgoing,
-      private: true,
-      sender: @assistant,
-      account: conversation.account,
-      inbox: conversation.inbox,
-      content: reason
-    )
-
-    # Trigger the bot handoff (sets status to open + dispatches events)
-    conversation.bot_handoff!
+  def trigger_handoff(_conversation, _reason)
+    # Signal to ResponseBuilderJob that handoff is requested
+    # The job will handle the actual bot_handoff! call outside the transaction
+    # to ensure after_commit callbacks work correctly (see PR.md for details)
+    Current.handoff_requested = true
   end
 
   # TODO: Future enhancement - Add team assignment capability

lib/current.rb

@@ -4,6 +4,7 @@ module Current
   thread_mattr_accessor :account_user
   thread_mattr_accessor :executed_by
   thread_mattr_accessor :contact
+  thread_mattr_accessor :handoff_requested
 
   def self.reset
     Current.user = nil
@@ -11,5 +12,6 @@ module Current
     Current.account_user = nil
     Current.executed_by = nil
     Current.contact = nil
+    Current.handoff_requested = nil
   end
 end

spec/enterprise/jobs/captain/conversation/response_builder_job_spec.rb

@@ -220,6 +220,189 @@ RSpec.describe Captain::Conversation::ResponseBuilderJob, type: :job do
     end
   end
 
+  describe 'handoff flow' do
+    let(:conversation) { create(:conversation, inbox: inbox, account: account, status: :pending) }
+    let(:mock_llm_chat_service) { instance_double(Captain::Llm::AssistantChatService) }
+
+    before do
+      create(:message, conversation: conversation, content: 'I need human help', message_type: :incoming)
+      allow(Captain::Llm::AssistantChatService).to receive(:new).and_return(mock_llm_chat_service)
+      allow(inbox).to receive(:captain_active?).and_return(true)
+    end
+
+    context 'when response requests handoff' do
+      before do
+        allow(mock_llm_chat_service).to receive(:generate_response)
+          .and_return({ 'response' => 'conversation_handoff' })
+      end
+
+      it 'triggers handoff and changes conversation status' do
+        described_class.perform_now(conversation, assistant)
+
+        conversation.reload
+        expect(conversation.status).to eq('open')
+      end
+
+      it 'creates handoff message' do
+        described_class.perform_now(conversation, assistant)
+
+        handoff_message = conversation.messages.outgoing.last
+        expect(handoff_message).not_to be_nil
+        expect(handoff_message.content).to include(I18n.t('conversations.captain.handoff'))
+      end
+
+      it 'does not increment response usage' do
+        described_class.perform_now(conversation, assistant)
+        account.reload
+        expect(account.usage_limits[:captain][:responses][:consumed]).to eq(0)
+      end
+    end
+
+    context 'when tool requests handoff via Current.handoff_requested' do
+      before do
+        allow(mock_llm_chat_service).to receive(:generate_response) do
+          # Simulate tool setting the flag during response generation
+          Current.handoff_requested = true
+          { 'response' => 'Processing your request...' }
+        end
+      end
+
+      it 'triggers handoff after transaction completes' do
+        described_class.perform_now(conversation, assistant)
+
+        conversation.reload
+        expect(conversation.status).to eq('open')
+      end
+
+      it 'creates handoff message' do
+        described_class.perform_now(conversation, assistant)
+
+        handoff_message = conversation.messages.outgoing.last
+        expect(handoff_message).not_to be_nil
+        expect(handoff_message.content).to include(I18n.t('conversations.captain.handoff'))
+      end
+
+      it 'does not create regular response message' do
+        described_class.perform_now(conversation, assistant)
+
+        # Should only have the handoff message, not the regular response
+        outgoing_messages = conversation.messages.outgoing
+        expect(outgoing_messages.count).to eq(1)
+        expect(outgoing_messages.last.content).not_to include('Processing your request...')
+      end
+
+      it 'does not increment response usage' do
+        described_class.perform_now(conversation, assistant)
+        account.reload
+        expect(account.usage_limits[:captain][:responses][:consumed]).to eq(0)
+      end
+    end
+
+    context 'when error occurs' do
+      let(:error) { StandardError.new('Something went wrong') }
+
+      before do
+        allow(mock_llm_chat_service).to receive(:generate_response).and_raise(error)
+        allow(ChatwootExceptionTracker).to receive(:new).and_call_original
+      end
+
+      it 'triggers handoff in ensure block' do
+        described_class.perform_now(conversation, assistant)
+
+        conversation.reload
+        expect(conversation.status).to eq('open')
+      end
+
+      it 'creates handoff message' do
+        described_class.perform_now(conversation, assistant)
+
+        handoff_message = conversation.messages.outgoing.last
+        expect(handoff_message).not_to be_nil
+        expect(handoff_message.content).to include(I18n.t('conversations.captain.handoff'))
+      end
+
+      it 'logs the error' do
+        expect(ChatwootExceptionTracker).to receive(:new)
+          .with(error, account: account)
+          .and_call_original
+
+        described_class.perform_now(conversation, assistant)
+      end
+    end
+
+    context 'when custom handoff message is configured' do
+      let(:custom_message) { 'Custom handoff message from config' }
+
+      before do
+        assistant.config = { 'handoff_message' => custom_message }
+        assistant.save!
+
+        allow(mock_llm_chat_service).to receive(:generate_response)
+          .and_return({ 'response' => 'conversation_handoff' })
+      end
+
+      it 'uses custom handoff message' do
+        described_class.perform_now(conversation, assistant)
+
+        handoff_message = conversation.messages.outgoing.last
+        expect(handoff_message.content).to eq(custom_message)
+      end
+    end
+
+    context 'when handoff happens outside transaction' do
+      before do
+        allow(mock_llm_chat_service).to receive(:generate_response)
+          .and_return({ 'response' => 'conversation_handoff' })
+      end
+
+      it 'ensures bot_handoff! is called outside transaction' do
+        # This verifies that bot_handoff! is called outside the transaction
+        # so that after_commit callbacks can see the status change correctly
+        allow(conversation).to receive(:bot_handoff!).and_call_original
+
+        described_class.perform_now(conversation, assistant)
+
+        # Verify status changed
+        expect(conversation.reload.status).to eq('open')
+      end
+
+      it 'maintains Current.executed_by during handoff' do
+        # Capture the executed_by value during bot_handoff! call
+        captured_executed_by = nil
+        allow(conversation).to receive(:bot_handoff!) do
+          captured_executed_by = Current.executed_by
+          conversation.status = :open
+          conversation.save!
+        end
+
+        described_class.perform_now(conversation, assistant)
+
+        expect(captured_executed_by).to eq(assistant)
+      end
+    end
+
+    context 'when Current values are cleaned up' do
+      before do
+        allow(mock_llm_chat_service).to receive(:generate_response)
+          .and_return({ 'response' => 'conversation_handoff' })
+      end
+
+      it 'resets Current.executed_by after job completes' do
+        described_class.perform_now(conversation, assistant)
+
+        expect(Current.executed_by).to be_nil
+      end
+
+      it 'resets Current.handoff_requested after job completes' do
+        Current.handoff_requested = true
+
+        described_class.perform_now(conversation, assistant)
+
+        expect(Current.handoff_requested).to be_nil
+      end
+    end
+  end
+
   describe 'job configuration' do
     it 'has retry_on configuration for retryable errors' do
       expect(described_class).to respond_to(:retry_on)

spec/enterprise/lib/captain/tools/handoff_tool_spec.rb

@@ -10,6 +10,12 @@ RSpec.describe Captain::Tools::HandoffTool, type: :model do
   let(:conversation) { create(:conversation, account: account, inbox: inbox, contact: contact) }
   let(:tool_context) { Struct.new(:state).new({ conversation: { id: conversation.id } }) }
 
+  after do
+    # Clean up Current values after each test
+    Current.executed_by = nil
+    Current.handoff_requested = nil
+  end
+
   describe '#description' do
     it 'returns the correct description' do
       expect(tool.description).to eq('Hand off the conversation to a human agent when unable to assist further')
@@ -29,38 +35,36 @@ RSpec.describe Captain::Tools::HandoffTool, type: :model do
   describe '#perform' do
     context 'when conversation exists' do
       context 'with reason provided' do
-        it 'creates a private note with reason and hands off conversation' do
-          reason = 'Customer needs specialized support'
+        before { conversation.update!(status: :pending) }
 
+        it 'returns success message with reason' do
+          reason = 'Customer needs specialized support'
+          result = tool.perform(tool_context, reason: reason)
+          expect(result).to eq("Conversation handed off to human support team (Reason: #{reason})")
+        end
+
+        it 'does not create any messages immediately' do
+          # Messages are created by ResponseBuilderJob, not the tool
           expect do
-            result = tool.perform(tool_context, reason: reason)
-            expect(result).to eq("Conversation handed off to human support team (Reason: #{reason})")
-          end.to change(Message, :count).by(1)
+            tool.perform(tool_context, reason: 'Test reason')
+          end.not_to change(Message, :count)
         end
 
-        it 'creates message with correct attributes' do
-          reason = 'Customer needs specialized support'
-          tool.perform(tool_context, reason: reason)
-
-          created_message = Message.last
-          expect(created_message.content).to eq(reason)
-          expect(created_message.message_type).to eq('outgoing')
-          expect(created_message.private).to be true
-          expect(created_message.sender).to eq(assistant)
-          expect(created_message.account).to eq(account)
-          expect(created_message.inbox).to eq(inbox)
-          expect(created_message.conversation).to eq(conversation)
-        end
-
-        it 'triggers bot handoff on conversation' do
-          # The tool finds the conversation by ID, so we need to mock the found conversation
-          found_conversation = Conversation.find(conversation.id)
-          scoped_conversations = Conversation.where(account_id: assistant.account_id)
-          allow(Conversation).to receive(:where).with(account_id: assistant.account_id).and_return(scoped_conversations)
-          allow(scoped_conversations).to receive(:find_by).with(id: conversation.id).and_return(found_conversation)
-          expect(found_conversation).to receive(:bot_handoff!)
-
+        it 'sets Current.handoff_requested flag' do
           tool.perform(tool_context, reason: 'Test reason')
+
+          expect(Current.handoff_requested).to be true
+        end
+
+        it 'does not change conversation status immediately' do
+          conversation.update(status: :pending)
+          expect(conversation.pending?).to be true
+
+          tool.perform(tool_context, reason: 'Customer needs specialized support')
+
+          conversation.reload
+          # Status should remain pending until ResponseBuilderJob processes the handoff
+          expect(conversation.pending?).to be true
         end
 
         it 'logs tool usage with reason' do
@@ -75,14 +79,24 @@ RSpec.describe Captain::Tools::HandoffTool, type: :model do
       end
 
       context 'without reason provided' do
-        it 'creates a private note with nil content and hands off conversation' do
-          expect do
-            result = tool.perform(tool_context)
-            expect(result).to eq('Conversation handed off to human support team')
-          end.to change(Message, :count).by(1)
+        before { conversation.update!(status: :pending) }
 
-          created_message = Message.last
-          expect(created_message.content).to be_nil
+        it 'returns success message without reason' do
+          result = tool.perform(tool_context)
+          expect(result).to eq('Conversation handed off to human support team')
+        end
+
+        it 'does not create any messages immediately' do
+          # Messages are created by ResponseBuilderJob, not the tool
+          expect do
+            tool.perform(tool_context)
+          end.not_to change(Message, :count)
+        end
+
+        it 'sets Current.handoff_requested flag' do
+          tool.perform(tool_context)
+
+          expect(Current.handoff_requested).to be true
         end
 
         it 'logs tool usage with default reason' do
@@ -95,31 +109,33 @@ RSpec.describe Captain::Tools::HandoffTool, type: :model do
         end
       end
 
-      context 'when handoff fails' do
-        before do
-          # Mock the conversation lookup and handoff failure
-          found_conversation = Conversation.find(conversation.id)
-          scoped_conversations = Conversation.where(account_id: assistant.account_id)
-          allow(Conversation).to receive(:where).with(account_id: assistant.account_id).and_return(scoped_conversations)
-          allow(scoped_conversations).to receive(:find_by).with(id: conversation.id).and_return(found_conversation)
-          allow(found_conversation).to receive(:bot_handoff!).and_raise(StandardError, 'Handoff error')
+      context 'when an error occurs' do
+        it 'returns error message' do
+          # Mock an error in trigger_handoff
+          allow(Current).to receive(:handoff_requested=).and_raise(StandardError, 'Unexpected error')
 
           exception_tracker = instance_double(ChatwootExceptionTracker)
           allow(ChatwootExceptionTracker).to receive(:new).and_return(exception_tracker)
           allow(exception_tracker).to receive(:capture_exception)
-        end
 
-        it 'returns error message' do
           result = tool.perform(tool_context, reason: 'Test')
           expect(result).to eq('Failed to handoff conversation')
+
+          # Unstub to allow cleanup
+          allow(Current).to receive(:handoff_requested=).and_call_original
         end
 
         it 'captures exception' do
           exception_tracker = instance_double(ChatwootExceptionTracker)
+          allow(Current).to receive(:handoff_requested=).and_raise(StandardError, 'Unexpected error')
+
           expect(ChatwootExceptionTracker).to receive(:new).with(instance_of(StandardError)).and_return(exception_tracker)
           expect(exception_tracker).to receive(:capture_exception)
 
           tool.perform(tool_context, reason: 'Test')
+
+          # Unstub to allow cleanup
+          allow(Current).to receive(:handoff_requested=).and_call_original
         end
       end
     end