diff --git a/enterprise/app/jobs/captain/conversation/response_builder_job.rb b/enterprise/app/jobs/captain/conversation/response_builder_job.rb index 268c6a3ee..5e7c5b3c2 100644 --- a/enterprise/app/jobs/captain/conversation/response_builder_job.rb +++ b/enterprise/app/jobs/captain/conversation/response_builder_job.rb @@ -45,11 +45,28 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob end def process_response - return unless conversation_pending? + # Check V2 before V1: error_response can set both signals at once when HandoffTool + # fired before the runner errored. V2 must win — running V1 on top would duplicate + # OOO and re-dispatch the bot_handoff event. + if v2_handoff_tool_fired? + if conversation_pending? + # HandoffTool flipped the flag without committing — its perform returned a + # failure string (e.g. "Conversation not found") before bot_handoff! ran. Fall + # back to a full V1 handoff so the customer still ends up with a human. + process_v1_handoff + else + # HandoffTool already opened the conversation inside the agent loop. All that's + # left is the customer-facing follow-up message. + process_v2_handoff + end + elsif v1_handoff_requested? + # V1 only signals via the response string — no state has been touched yet. If + # the conversation isn't pending anymore, a human took over mid-run; bail out + # rather than posting a stale handoff message on top of their reply. + return unless conversation_pending? - if handoff_requested? - process_action('handoff') - else + process_v1_handoff + elsif conversation_pending? ActiveRecord::Base.transaction do create_messages Rails.logger.info("[CAPTAIN][ResponseBuilderJob] Incrementing response usage for #{account.id}") @@ -84,18 +101,27 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob Captain::OpenAiMessageBuilderService.new(message: message).generate_content end - def handoff_requested? + def v1_handoff_requested? @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! - send_out_of_office_message_if_applicable - end + def v2_handoff_tool_fired? + @response['handoff_tool_called'] + end + + def process_v1_handoff + I18n.with_locale(@assistant.account.locale) do + create_handoff_message + @conversation.bot_handoff! + send_out_of_office_message_if_applicable + end + end + + def process_v2_handoff + # HandoffTool already ran bot_handoff! + OOO inside the agent loop. Preserve + # waiting_since so this message doesn't clear the timestamp it left in place. + I18n.with_locale(@assistant.account.locale) do + create_handoff_message(preserve_waiting_since: true) end end @@ -107,9 +133,10 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob ::MessageTemplates::Template::OutOfOffice.perform_if_applicable(@conversation) end - def create_handoff_message + def create_handoff_message(preserve_waiting_since: false) create_outgoing_message( - @assistant.config['handoff_message'].presence || I18n.t('conversations.captain.handoff') + @assistant.config['handoff_message'].presence || I18n.t('conversations.captain.handoff'), + preserve_waiting_since: preserve_waiting_since ) end @@ -122,7 +149,7 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob raise ArgumentError, 'Message content cannot be blank' if content.blank? end - def create_outgoing_message(message_content, agent_name: nil) + def create_outgoing_message(message_content, agent_name: nil, preserve_waiting_since: false) additional_attrs = {} additional_attrs[:agent_name] = agent_name if agent_name.present? @@ -132,13 +159,14 @@ class Captain::Conversation::ResponseBuilderJob < ApplicationJob inbox_id: inbox.id, sender: @assistant, content: message_content, - additional_attributes: additional_attrs + additional_attributes: additional_attrs, + preserve_waiting_since: preserve_waiting_since ) end def handle_error(error) log_error(error) - process_action('handoff') if conversation_pending? + process_v1_handoff if conversation_pending? true end diff --git a/enterprise/app/services/captain/assistant/agent_runner_service.rb b/enterprise/app/services/captain/assistant/agent_runner_service.rb index a58084960..21a9c331e 100644 --- a/enterprise/app/services/captain/assistant/agent_runner_service.rb +++ b/enterprise/app/services/captain/assistant/agent_runner_service.rb @@ -24,6 +24,7 @@ class Captain::Assistant::AgentRunnerService @conversation = conversation @callbacks = callbacks @source = source + @handoff_tool_called = false end def generate_response(message_history: []) @@ -98,13 +99,15 @@ class Captain::Assistant::AgentRunnerService output = result.output response = output.is_a?(Hash) ? output.with_indifferent_access : { 'response' => output.to_s, 'reasoning' => 'Processed by agent' } response['agent_name'] = result.context&.dig(:current_agent) + response['handoff_tool_called'] = result.context&.dig(:captain_v2_handoff_tool_called) || false response end def error_response(error_message) { 'response' => 'conversation_handoff', - 'reasoning' => "Error occurred: #{error_message}" + 'reasoning' => "Error occurred: #{error_message}", + 'handoff_tool_called' => @handoff_tool_called } end @@ -175,16 +178,18 @@ class Captain::Assistant::AgentRunnerService end def add_usage_metadata_callback(runner) - return runner unless ChatwootApp.otel_enabled? - handoff_tool_name = Captain::Tools::HandoffTool.new(@assistant).name + # Tool tracking always runs — process_response in the job consumes the resulting + # handoff_tool_called flag regardless of whether OTEL is enabled. runner.on_tool_complete do |tool_name, _tool_result, context_wrapper| track_handoff_usage(tool_name, handoff_tool_name, context_wrapper) end - runner.on_run_complete do |_agent_name, _result, context_wrapper| - write_credits_used_metadata(context_wrapper) + if ChatwootApp.otel_enabled? + runner.on_run_complete do |_agent_name, _result, context_wrapper| + write_credits_used_metadata(context_wrapper) + end end runner end @@ -193,15 +198,17 @@ class Captain::Assistant::AgentRunnerService return unless context_wrapper&.context return unless tool_name.to_s == handoff_tool_name + # Mirror the flag onto the instance so error_response can surface it even when + # the runner raises before returning a result (the context is unreachable then). context_wrapper.context[:captain_v2_handoff_tool_called] = true + @handoff_tool_called = true end def write_credits_used_metadata(context_wrapper) root_span = context_wrapper&.context&.dig(:__otel_tracing, :root_span) return unless root_span - credit_used = !context_wrapper.context[:captain_v2_handoff_tool_called] - root_span.set_attribute(format(ATTR_LANGFUSE_METADATA, 'credit_used'), credit_used.to_s) + root_span.set_attribute(format(ATTR_LANGFUSE_METADATA, 'credit_used'), @handoff_tool_called ? 'false' : 'true') end def runner 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 1baccf5c7..548b84992 100644 --- a/spec/enterprise/jobs/captain/conversation/response_builder_job_spec.rb +++ b/spec/enterprise/jobs/captain/conversation/response_builder_job_spec.rb @@ -101,6 +101,90 @@ RSpec.describe Captain::Conversation::ResponseBuilderJob, type: :job do end end + context 'when captain_v2 handoff tool fires during agent execution' do + before do + allow(account).to receive(:feature_enabled?).and_return(false) + allow(account).to receive(:feature_enabled?).with('captain_integration_v2').and_return(true) + end + + it 'creates a public handoff message visible to the customer' do + allow(mock_agent_runner_service).to receive(:generate_response) do + conversation.update!(status: :open) + { 'response' => 'Let me connect you', 'handoff_tool_called' => true } + end + + described_class.perform_now(conversation, assistant) + + public_messages = conversation.messages.outgoing.where(private: false) + expect(public_messages.count).to eq(1) + expect(public_messages.last.content).to eq(I18n.t('conversations.captain.handoff')) + end + + it 'does not call bot_handoff! again when conversation is already open' do + allow(mock_agent_runner_service).to receive(:generate_response) do + conversation.update!(status: :open) + { 'response' => 'Let me connect you', 'handoff_tool_called' => true } + end + + expect(conversation).not_to receive(:bot_handoff!) + + described_class.perform_now(conversation, assistant) + end + + it 'does not create a duplicate out of office message' do + allow(mock_agent_runner_service).to receive(:generate_response) do + conversation.update!(status: :open) + { 'response' => 'Let me connect you', 'handoff_tool_called' => true } + end + + described_class.perform_now(conversation, assistant) + + expect(conversation.messages.template.count).to eq(0) + end + + it 'preserves waiting_since when HandoffTool already called bot_handoff!' do + original_waiting_since = 5.minutes.ago + conversation.update!(waiting_since: original_waiting_since) + + allow(mock_agent_runner_service).to receive(:generate_response) do + conversation.update!(status: :open, waiting_since: original_waiting_since) + { 'response' => 'Let me connect you', 'handoff_tool_called' => true } + end + + described_class.perform_now(conversation, assistant) + + expect(conversation.reload.waiting_since).to be_within(1.second).of(original_waiting_since) + end + + it 'does not hand off when handoff_tool_called is false' do + allow(mock_agent_runner_service).to receive(:generate_response).and_return({ + 'response' => 'Hi! How can I help you?', + 'handoff_tool_called' => false + }) + + described_class.perform_now(conversation, assistant) + + expect(conversation.messages.outgoing.count).to eq(1) + expect(conversation.messages.last.content).to eq('Hi! How can I help you?') + expect(conversation.reload.status).to eq('pending') + end + + it 'falls back to a full V1 handoff when HandoffTool fired but failed to commit' do + allow(mock_agent_runner_service).to receive(:generate_response).and_return({ + 'response' => 'I tried to hand off', + 'handoff_tool_called' => true + }) + + described_class.perform_now(conversation, assistant) + + conversation.reload + expect(conversation.status).to eq('open') + public_messages = conversation.messages.outgoing.where(private: false) + expect(public_messages.count).to eq(1) + expect(public_messages.last.content).to eq(I18n.t('conversations.captain.handoff')) + end + end + # Regression (PR #13417): wrapping create_handoff_message and bot_handoff! in the # same transaction defers the message's after_create_commit until commit, at which # point it clears waiting_since (bot_response). The handoff path must stay outside @@ -116,13 +200,15 @@ RSpec.describe Captain::Conversation::ResponseBuilderJob, type: :job do end it 'sets waiting_since to approximately the handoff time' do - freeze_time do - described_class.perform_now(conversation, assistant) + # Don't use freeze_time here: we need a real gap between the seeded waiting_since + # and Time.current, otherwise "preserved" and "reset" both look identical. + conversation.update!(waiting_since: 10.minutes.ago) - conversation.reload - expect(conversation.status).to eq('open') - expect(conversation.waiting_since).to be_within(1.second).of(Time.current) - end + described_class.perform_now(conversation, assistant) + + conversation.reload + expect(conversation.status).to eq('open') + expect(conversation.waiting_since).to be_within(5.seconds).of(Time.current) end it 'preserves waiting_since so a human reply consumes it for reply_time tracking' do diff --git a/spec/enterprise/services/captain/assistant/agent_runner_service_spec.rb b/spec/enterprise/services/captain/assistant/agent_runner_service_spec.rb index e232be19d..d6e57e710 100644 --- a/spec/enterprise/services/captain/assistant/agent_runner_service_spec.rb +++ b/spec/enterprise/services/captain/assistant/agent_runner_service_spec.rb @@ -10,7 +10,7 @@ RSpec.describe Captain::Assistant::AgentRunnerService do let(:assistant) { create(:captain_assistant, account: account) } let(:scenario) { create(:captain_scenario, assistant: assistant, enabled: true) } - let(:mock_runner) { instance_double(Agents::Runner) } + let(:mock_runner) { instance_double(Agents::AgentRunner) } let(:mock_agent) { instance_double(Agents::Agent) } let(:mock_scenario_agent) { instance_double(Agents::Agent) } let(:mock_result) { instance_double(Agents::RunResult, output: { 'response' => 'Test response' }, context: nil) } @@ -31,6 +31,8 @@ RSpec.describe Captain::Assistant::AgentRunnerService do allow(scenario).to receive(:agent).and_return(mock_scenario_agent) allow(Agents::Runner).to receive(:with_agents).and_return(mock_runner) allow(mock_runner).to receive(:run).and_return(mock_result) + allow(mock_runner).to receive(:on_tool_complete).and_return(mock_runner) + allow(mock_runner).to receive(:on_run_complete).and_return(mock_runner) allow(mock_agent).to receive(:register_handoffs) allow(mock_scenario_agent).to receive(:register_handoffs) end @@ -165,7 +167,24 @@ RSpec.describe Captain::Assistant::AgentRunnerService do it 'processes and formats agent result' do result = service.generate_response(message_history: message_history) - expect(result).to eq({ 'response' => 'Test response', 'agent_name' => nil }) + expect(result).to eq({ 'response' => 'Test response', 'agent_name' => nil, 'handoff_tool_called' => false }) + end + + context 'when handoff tool was called during agent execution' do + let(:runner_context) { { captain_v2_handoff_tool_called: true } } + let(:mock_result) do + instance_double(Agents::RunResult, output: { 'response' => 'Let me connect you' }, context: runner_context) + end + + it 'includes handoff_tool_called flag in response' do + result = service.generate_response(message_history: message_history) + + expect(result).to eq({ + 'response' => 'Let me connect you', + 'agent_name' => nil, + 'handoff_tool_called' => true + }) + end end context 'when no scenarios are enabled' do @@ -192,7 +211,8 @@ RSpec.describe Captain::Assistant::AgentRunnerService do expect(result).to eq({ 'response' => 'Simple string response', 'reasoning' => 'Processed by agent', - 'agent_name' => nil + 'agent_name' => nil, + 'handoff_tool_called' => false }) end end @@ -214,7 +234,8 @@ RSpec.describe Captain::Assistant::AgentRunnerService do expect(result).to eq({ 'response' => 'conversation_handoff', - 'reasoning' => 'Error occurred: Test error' + 'reasoning' => 'Error occurred: Test error', + 'handoff_tool_called' => false }) end @@ -235,7 +256,32 @@ RSpec.describe Captain::Assistant::AgentRunnerService do expect(result).to eq({ 'response' => 'conversation_handoff', - 'reasoning' => 'Error occurred: Test error' + 'reasoning' => 'Error occurred: Test error', + 'handoff_tool_called' => false + }) + end + end + + context 'when HandoffTool fired before the runner errored' do + # The stubbed runner never invokes the on_tool_complete callback, so we call + # track_handoff_usage directly to simulate the flag being set before the raise. + before do + allow(mock_runner).to receive(:run) do + service.send(:track_handoff_usage, + Captain::Tools::HandoffTool.new(assistant).name, + Captain::Tools::HandoffTool.new(assistant).name, + Struct.new(:context).new({})) + raise error + end + end + + it 'surfaces handoff_tool_called in error_response so the job routes to the V2 path' do + result = service.generate_response(message_history: message_history) + + expect(result).to eq({ + 'response' => 'conversation_handoff', + 'reasoning' => 'Error occurred: Test error', + 'handoff_tool_called' => true }) end end @@ -479,6 +525,38 @@ RSpec.describe Captain::Assistant::AgentRunnerService do run_complete_callback.call('assistant', nil, context_wrapper) end + it 'registers handoff tracking callback when OTEL is disabled' do + service = described_class.new(assistant: assistant, conversation: conversation) + runner = instance_double(Agents::AgentRunner) + tool_complete_callback = nil + + allow(ChatwootApp).to receive(:otel_enabled?).and_return(false) + allow(runner).to receive(:on_tool_complete) do |&block| + tool_complete_callback = block + runner + end + + service.send(:add_usage_metadata_callback, runner) + + context_wrapper = Struct.new(:context).new({}) + + expect(tool_complete_callback).not_to be_nil + tool_complete_callback.call(Captain::Tools::HandoffTool.new(assistant).name, 'ok', context_wrapper) + + expect(context_wrapper.context[:captain_v2_handoff_tool_called]).to be true + end + + it 'does not register OTEL run callback when OTEL is disabled' do + service = described_class.new(assistant: assistant, conversation: conversation) + runner = instance_double(Agents::AgentRunner) + + allow(ChatwootApp).to receive(:otel_enabled?).and_return(false) + allow(runner).to receive(:on_tool_complete).and_return(runner) + expect(runner).not_to receive(:on_run_complete) + + service.send(:add_usage_metadata_callback, runner) + end + it 'sets credit_used=true when handoff tool is not used' do service = described_class.new(assistant: assistant, conversation: conversation) runner = instance_double(Agents::AgentRunner)