Improve exception handling and add class-level handler for GenerationJob

lib/active_agent/concerns/rescue.rb

@@ -13,6 +13,45 @@ module Rescue
     include ActiveSupport::Rescuable
 
     class_methods do
+      # Handles exceptions raised during GenerationJob execution.
+      #
+      # Called by GenerationJob#handle_exception_with_agent_class as a
+      # class-level fallback when no instance is available to handle the error.
+      # Logs the exception and re-raises to preserve job failure semantics.
+      #
+      # @param exception [Exception] the exception to handle
+      # @raise [Exception] re-raises the exception after logging
+      # @return [void]
+      def handle_exception(exception)
+        rescue_logger.error "[#{name}] #{exception.class}: #{exception.message}"
+        rescue_logger.error exception.backtrace&.first(10)&.join("\n") if exception.backtrace
+        raise exception
+      end
+
+      private
+
+      # Returns the logger to use for class-level exception handling.
+      #
+      # Prefers the including class's logger, then ActiveAgent::Base.logger,
+      # then Rails.logger if available, and finally falls back to a standard
+      # Ruby Logger to $stderr.
+      #
+      # @return [#error] a logger-like object responding to `#error`
+      def rescue_logger
+        if respond_to?(:logger) && (current_logger = logger)
+          current_logger
+        elsif defined?(ActiveAgent::Base) &&
+              ActiveAgent::Base.respond_to?(:logger) &&
+              (base_logger = ActiveAgent::Base.logger)
+          base_logger
+        elsif defined?(Rails) && Rails.respond_to?(:logger) && Rails.logger
+          Rails.logger
+        else
+          require "logger"
+          @_rescue_logger ||= Logger.new($stderr)
+        end
+      end
+
       # Finds and instruments the rescue handler for an exception.
       #
       # @param exception [Exception] the exception to handle

lib/active_agent/providers/concerns/exception_handler.rb

@@ -55,6 +55,7 @@ def with_exception_handling(&block)
         yield
       rescue => exception
         rescue_with_handler(exception) || raise
+        nil # Discard handler return value to prevent polluting raw_response
       end
 
       # Bubbles up exceptions to the Agent's rescue_from if a handler is defined.

test/features/rescue_test.rb

@@ -211,4 +211,50 @@ def buggy_handler(exception)
 
     assert_equal "Handler error", error.message
   end
+
+  # Class-level handle_exception tests for GenerationJob integration
+  test "handle_exception class method logs and re-raises exception" do
+    exception = CustomError.new("Test job error")
+    exception.set_backtrace([ "line1", "line2", "line3" ])
+
+    # Should re-raise the exception after logging
+    error = assert_raises(CustomError) do
+      TestAgent.handle_exception(exception)
+    end
+
+    assert_same exception, error
+  end
+
+  test "handle_exception uses rescue_logger for logging" do
+    # Create a mock logger to capture log messages
+    log_messages = []
+    mock_logger = Object.new
+    mock_logger.define_singleton_method(:error) { |msg| log_messages << msg }
+
+    # Temporarily replace the logger
+    TestAgent.stub(:rescue_logger, mock_logger) do
+      exception = CustomError.new("Logged error")
+      exception.set_backtrace([ "backtrace_line_1", "backtrace_line_2" ])
+
+      assert_raises(CustomError) do
+        TestAgent.handle_exception(exception)
+      end
+    end
+
+    assert_equal 2, log_messages.size
+    assert_includes log_messages[0], "CustomError"
+    assert_includes log_messages[0], "Logged error"
+    assert_includes log_messages[1], "backtrace_line_1"
+  end
+
+  test "handle_exception handles exceptions without backtrace" do
+    exception = CustomError.new("No backtrace")
+    # Don't set backtrace
+
+    error = assert_raises(CustomError) do
+      TestAgent.handle_exception(exception)
+    end
+
+    assert_same exception, error
+  end
 end

test/generation_job_test.rb

@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+require "test_helper"
+
+class GenerationJobTest < ActiveSupport::TestCase
+  class TestJobError < StandardError; end
+
+  class FailingAgent < ApplicationAgent
+    class << self
+      attr_accessor :exception_handled
+    end
+
+    def self.handle_exception(exception)
+      self.exception_handled = exception
+      super
+    end
+
+    def failing_action
+      raise TestJobError, "Job failed"
+    end
+  end
+
+  setup do
+    FailingAgent.exception_handled = nil
+  end
+
+  test "handle_exception_with_agent_class calls agent class handle_exception" do
+    job = ActiveAgent::GenerationJob.new
+    job.arguments = [ "GenerationJobTest::FailingAgent", "failing_action", "prompt_now", { args: [] } ]
+
+    exception = TestJobError.new("Test error")
+
+    # The job should re-raise after the agent logs it
+    error = assert_raises(TestJobError) do
+      job.send(:handle_exception_with_agent_class, exception)
+    end
+
+    assert_same exception, error
+    assert_same exception, FailingAgent.exception_handled
+  end
+
+  test "handle_exception_with_agent_class re-raises when no agent class" do
+    job = ActiveAgent::GenerationJob.new
+    job.arguments = []
+
+    exception = TestJobError.new("No agent class")
+
+    error = assert_raises(TestJobError) do
+      job.send(:handle_exception_with_agent_class, exception)
+    end
+
+    assert_same exception, error
+  end
+end

test/providers/concerns/exception_handler_test.rb

@@ -53,7 +53,7 @@ def perform_operation
         assert_equal 1, provider.call_count
       end
 
-      test "calls exception handler when configured" do
+      test "calls exception handler when configured and returns nil" do
         handled_exception = nil
         handler = ->(exception) { handled_exception = exception; "handled" }
 
@@ -62,7 +62,8 @@ def perform_operation
 
         result = provider.perform_operation
 
-        assert_equal "handled", result
+        # Returns nil to prevent handler return value from polluting raw_response
+        assert_nil result
         assert_instance_of TestError, handled_exception
         assert_equal "test error", handled_exception.message
       end
@@ -90,24 +91,28 @@ def perform_operation
         # Test with TestError
         provider.error_to_raise = TestError.new("test")
         result = provider.perform_operation
-        assert_equal "handled ActiveAgent::Providers::ExceptionHandlerTest::TestError", result
+        assert_nil result # Returns nil, not handler return value
 
         # Test with CustomError
         provider.error_to_raise = CustomError.new("custom")
         result = provider.perform_operation
-        assert_equal "handled ActiveAgent::Providers::ExceptionHandlerTest::CustomError", result
+        assert_nil result # Returns nil, not handler return value
 
         assert_equal 2, handled_exceptions.size
+        assert_instance_of TestError, handled_exceptions[0]
+        assert_instance_of CustomError, handled_exceptions[1]
       end
 
       test "exception handler receives actual exception object" do
-        provider = MockProvider.new(exception_handler: ->(e) { e })
+        received_exception = nil
+        provider = MockProvider.new(exception_handler: ->(e) { received_exception = e })
         original_error = TestError.new("original message")
         provider.error_to_raise = original_error
 
         result = provider.perform_operation
 
-        assert_same original_error, result
+        assert_nil result # Returns nil after handling
+        assert_same original_error, received_exception
       end
 
       test "rescue_with_handler returns handler result" do
@@ -127,6 +132,21 @@ def perform_operation
 
         assert_nil result
       end
+
+      test "with_exception_handling returns nil to prevent raw_response pollution" do
+        # This test verifies the fix for issue #305
+        # When an exception is handled, with_exception_handling should return nil
+        # to prevent the handler's return value from becoming raw_response
+        handler_return_value = "some truthy value that would cause issues"
+        provider = MockProvider.new(exception_handler: ->(_) { handler_return_value })
+        provider.error_to_raise = TestError.new("api error")
+
+        result = provider.perform_operation
+
+        # Even though the handler returns a truthy value, with_exception_handling
+        # should discard it and return nil to avoid polluting raw_response
+        assert_nil result, "with_exception_handling should return nil after handling an exception"
+      end
     end
   end
 end