From 9bac26986bd810baa2f1b1f399f4602281e261e6 Mon Sep 17 00:00:00 2001 From: Justin Bowen Date: Tue, 24 Feb 2026 13:02:24 -0800 Subject: [PATCH] Improve exception handling and add class-level handler for GenerationJob MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses issues raised in PRs #309 and #310 with proper test coverage. ## Changes ### Fix raw_response pollution (Issue #305) - with_exception_handling now explicitly returns nil after handling an exception - Prevents handler return values from becoming raw_response and causing secondary NoMethodError when calling methods like `usage` on it ### Add class-level handle_exception for GenerationJob (Issue #306) - Add handle_exception class method to ActiveAgent::Rescue concern - Called by GenerationJob#handle_exception_with_agent_class as fallback - Uses configurable logger (rescue_logger) instead of hardcoded Rails.logger - Re-raises exception after logging to preserve job failure semantics - Logger fallback chain: class logger → ActiveAgent::Base.logger → Rails.logger → $stderr ### Test Coverage - Update exception_handler_test.rb to expect nil return after handling - Add tests for class-level handle_exception method - Add test for GenerationJob integration - Add test verifying raw_response pollution fix Fixes #305, #306 Supersedes #309, #310 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Co-Authored-By: TheRealNeil <3514122+TheRealNeil@users.noreply.github.com> --- lib/active_agent/concerns/rescue.rb | 39 ++++++++++++++ .../providers/concerns/exception_handler.rb | 1 + test/features/rescue_test.rb | 46 ++++++++++++++++ test/generation_job_test.rb | 54 +++++++++++++++++++ .../concerns/exception_handler_test.rb | 32 ++++++++--- 5 files changed, 166 insertions(+), 6 deletions(-) create mode 100644 test/generation_job_test.rb diff --git a/lib/active_agent/concerns/rescue.rb b/lib/active_agent/concerns/rescue.rb index 5f3938ba..090f1659 100644 --- a/lib/active_agent/concerns/rescue.rb +++ b/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 diff --git a/lib/active_agent/providers/concerns/exception_handler.rb b/lib/active_agent/providers/concerns/exception_handler.rb index 97e4cf54..9abb1d08 100644 --- a/lib/active_agent/providers/concerns/exception_handler.rb +++ b/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. diff --git a/test/features/rescue_test.rb b/test/features/rescue_test.rb index c6b9f443..da059ce0 100644 --- a/test/features/rescue_test.rb +++ b/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 diff --git a/test/generation_job_test.rb b/test/generation_job_test.rb new file mode 100644 index 00000000..858d3dab --- /dev/null +++ b/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 diff --git a/test/providers/concerns/exception_handler_test.rb b/test/providers/concerns/exception_handler_test.rb index c67ef972..dc055a8f 100644 --- a/test/providers/concerns/exception_handler_test.rb +++ b/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