fix(captain): display handoff message to customer in V2 flow (#13885)
HandoffTool changes conversation status but only posts a private note.
ResponseBuilderJob now detects the tool flag and creates the public
handoff message that was previously only shown in V1.
# Pull Request Template
## Description
Captain V2 was silently forwarding conversations to humans without
showing a handoff message to the customer. The conversation appeared to
just stop
responding.
Root cause: In V2, HandoffTool calls bot_handoff! during agent
execution, which changes conversation status from pending to open. By
the time control returns
to ResponseBuilderJob#process_response, the conversation_pending? guard
returns early - skipping create_handoff_message entirely. The V1 flow
didn't have this
problem because AssistantChatService just returns a string token
(conversation_handoff) and lets ResponseBuilderJob handle everything.
What changed:
1. AgentRunnerService now surfaces the handoff_tool_called flag (already
tracked internally for usage metadata) in its response hash.
2. ResponseBuilderJob#handoff_requested? detects handoffs from both V1
(response token) and V2 (tool flag).
3. ResponseBuilderJob#process_response checks handoff_requested? before
the conversation_pending? guard, so V2 handoffs are processed even when
the status has
already changed.
4. ResponseBuilderJob#process_action('handoff') captures
conversation_pending? before calling bot_handoff! and uses that snapshot
to guard both bot_handoff!
and the OOO message - preventing double-execution when V2's HandoffTool
already ran them.
New V2 handoff flow:
AgentRunnerService
→ agent calls HandoffTool (creates private note, calls bot_handoff!)
→ returns response with handoff_tool_called: true
ResponseBuilderJob#process_response
→ handoff_requested? detects the flag
→ process_action('handoff')
→ create_handoff_message (public message for customer)
→ bot_handoff! skipped (conversation_pending? is false)
→ OOO skipped (conversation_pending? is false)
Fixes #13881
## Type of change
Please delete options that are not relevant.
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality not to work as expected)
- [ ] This change requires a documentation update
## How Has This Been Tested?
- Update existing response_builder_job_spec.rb covering the V2 handoff
path, V2 normal response path, and V1 regression
- Updated existing agent_runner_service_spec.rb expectations for the new
handoff_tool_called key and added a context for when the flag is true
## Checklist:
- [x] My code follows the style guidelines of this project
- [x] I have performed a self-review of my code
- [ ] I have commented on my code, particularly in hard-to-understand
areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] New and existing unit tests pass locally with my changes
- [x] Any dependent changes have been merged and published in downstream
modules
---------
Co-authored-by: Aakash Bakhle <48802744+aakashb95@users.noreply.github.com>
Co-authored-by: aakashb95 <aakashbakhle@gmail.com>
This commit is contained in:
zip-fa
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user