Skip to content

Commit 9bac269

Browse files
TonsOfFunclaudeTheRealNeil
committed
Improve exception handling and add class-level handler for GenerationJob
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 <noreply@anthropic.com> Co-Authored-By: TheRealNeil <3514122+TheRealNeil@users.noreply.github.com>
1 parent 4d02fc5 commit 9bac269

File tree

5 files changed

+166
-6
lines changed

5 files changed

+166
-6
lines changed

lib/active_agent/concerns/rescue.rb

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,45 @@ module Rescue
1313
include ActiveSupport::Rescuable
1414

1515
class_methods do
16+
# Handles exceptions raised during GenerationJob execution.
17+
#
18+
# Called by GenerationJob#handle_exception_with_agent_class as a
19+
# class-level fallback when no instance is available to handle the error.
20+
# Logs the exception and re-raises to preserve job failure semantics.
21+
#
22+
# @param exception [Exception] the exception to handle
23+
# @raise [Exception] re-raises the exception after logging
24+
# @return [void]
25+
def handle_exception(exception)
26+
rescue_logger.error "[#{name}] #{exception.class}: #{exception.message}"
27+
rescue_logger.error exception.backtrace&.first(10)&.join("\n") if exception.backtrace
28+
raise exception
29+
end
30+
31+
private
32+
33+
# Returns the logger to use for class-level exception handling.
34+
#
35+
# Prefers the including class's logger, then ActiveAgent::Base.logger,
36+
# then Rails.logger if available, and finally falls back to a standard
37+
# Ruby Logger to $stderr.
38+
#
39+
# @return [#error] a logger-like object responding to `#error`
40+
def rescue_logger
41+
if respond_to?(:logger) && (current_logger = logger)
42+
current_logger
43+
elsif defined?(ActiveAgent::Base) &&
44+
ActiveAgent::Base.respond_to?(:logger) &&
45+
(base_logger = ActiveAgent::Base.logger)
46+
base_logger
47+
elsif defined?(Rails) && Rails.respond_to?(:logger) && Rails.logger
48+
Rails.logger
49+
else
50+
require "logger"
51+
@_rescue_logger ||= Logger.new($stderr)
52+
end
53+
end
54+
1655
# Finds and instruments the rescue handler for an exception.
1756
#
1857
# @param exception [Exception] the exception to handle

lib/active_agent/providers/concerns/exception_handler.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ def with_exception_handling(&block)
5555
yield
5656
rescue => exception
5757
rescue_with_handler(exception) || raise
58+
nil # Discard handler return value to prevent polluting raw_response
5859
end
5960

6061
# Bubbles up exceptions to the Agent's rescue_from if a handler is defined.

test/features/rescue_test.rb

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,4 +211,50 @@ def buggy_handler(exception)
211211

212212
assert_equal "Handler error", error.message
213213
end
214+
215+
# Class-level handle_exception tests for GenerationJob integration
216+
test "handle_exception class method logs and re-raises exception" do
217+
exception = CustomError.new("Test job error")
218+
exception.set_backtrace([ "line1", "line2", "line3" ])
219+
220+
# Should re-raise the exception after logging
221+
error = assert_raises(CustomError) do
222+
TestAgent.handle_exception(exception)
223+
end
224+
225+
assert_same exception, error
226+
end
227+
228+
test "handle_exception uses rescue_logger for logging" do
229+
# Create a mock logger to capture log messages
230+
log_messages = []
231+
mock_logger = Object.new
232+
mock_logger.define_singleton_method(:error) { |msg| log_messages << msg }
233+
234+
# Temporarily replace the logger
235+
TestAgent.stub(:rescue_logger, mock_logger) do
236+
exception = CustomError.new("Logged error")
237+
exception.set_backtrace([ "backtrace_line_1", "backtrace_line_2" ])
238+
239+
assert_raises(CustomError) do
240+
TestAgent.handle_exception(exception)
241+
end
242+
end
243+
244+
assert_equal 2, log_messages.size
245+
assert_includes log_messages[0], "CustomError"
246+
assert_includes log_messages[0], "Logged error"
247+
assert_includes log_messages[1], "backtrace_line_1"
248+
end
249+
250+
test "handle_exception handles exceptions without backtrace" do
251+
exception = CustomError.new("No backtrace")
252+
# Don't set backtrace
253+
254+
error = assert_raises(CustomError) do
255+
TestAgent.handle_exception(exception)
256+
end
257+
258+
assert_same exception, error
259+
end
214260
end

test/generation_job_test.rb

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
# frozen_string_literal: true
2+
3+
require "test_helper"
4+
5+
class GenerationJobTest < ActiveSupport::TestCase
6+
class TestJobError < StandardError; end
7+
8+
class FailingAgent < ApplicationAgent
9+
class << self
10+
attr_accessor :exception_handled
11+
end
12+
13+
def self.handle_exception(exception)
14+
self.exception_handled = exception
15+
super
16+
end
17+
18+
def failing_action
19+
raise TestJobError, "Job failed"
20+
end
21+
end
22+
23+
setup do
24+
FailingAgent.exception_handled = nil
25+
end
26+
27+
test "handle_exception_with_agent_class calls agent class handle_exception" do
28+
job = ActiveAgent::GenerationJob.new
29+
job.arguments = [ "GenerationJobTest::FailingAgent", "failing_action", "prompt_now", { args: [] } ]
30+
31+
exception = TestJobError.new("Test error")
32+
33+
# The job should re-raise after the agent logs it
34+
error = assert_raises(TestJobError) do
35+
job.send(:handle_exception_with_agent_class, exception)
36+
end
37+
38+
assert_same exception, error
39+
assert_same exception, FailingAgent.exception_handled
40+
end
41+
42+
test "handle_exception_with_agent_class re-raises when no agent class" do
43+
job = ActiveAgent::GenerationJob.new
44+
job.arguments = []
45+
46+
exception = TestJobError.new("No agent class")
47+
48+
error = assert_raises(TestJobError) do
49+
job.send(:handle_exception_with_agent_class, exception)
50+
end
51+
52+
assert_same exception, error
53+
end
54+
end

test/providers/concerns/exception_handler_test.rb

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ def perform_operation
5353
assert_equal 1, provider.call_count
5454
end
5555

56-
test "calls exception handler when configured" do
56+
test "calls exception handler when configured and returns nil" do
5757
handled_exception = nil
5858
handler = ->(exception) { handled_exception = exception; "handled" }
5959

@@ -62,7 +62,8 @@ def perform_operation
6262

6363
result = provider.perform_operation
6464

65-
assert_equal "handled", result
65+
# Returns nil to prevent handler return value from polluting raw_response
66+
assert_nil result
6667
assert_instance_of TestError, handled_exception
6768
assert_equal "test error", handled_exception.message
6869
end
@@ -90,24 +91,28 @@ def perform_operation
9091
# Test with TestError
9192
provider.error_to_raise = TestError.new("test")
9293
result = provider.perform_operation
93-
assert_equal "handled ActiveAgent::Providers::ExceptionHandlerTest::TestError", result
94+
assert_nil result # Returns nil, not handler return value
9495

9596
# Test with CustomError
9697
provider.error_to_raise = CustomError.new("custom")
9798
result = provider.perform_operation
98-
assert_equal "handled ActiveAgent::Providers::ExceptionHandlerTest::CustomError", result
99+
assert_nil result # Returns nil, not handler return value
99100

100101
assert_equal 2, handled_exceptions.size
102+
assert_instance_of TestError, handled_exceptions[0]
103+
assert_instance_of CustomError, handled_exceptions[1]
101104
end
102105

103106
test "exception handler receives actual exception object" do
104-
provider = MockProvider.new(exception_handler: ->(e) { e })
107+
received_exception = nil
108+
provider = MockProvider.new(exception_handler: ->(e) { received_exception = e })
105109
original_error = TestError.new("original message")
106110
provider.error_to_raise = original_error
107111

108112
result = provider.perform_operation
109113

110-
assert_same original_error, result
114+
assert_nil result # Returns nil after handling
115+
assert_same original_error, received_exception
111116
end
112117

113118
test "rescue_with_handler returns handler result" do
@@ -127,6 +132,21 @@ def perform_operation
127132

128133
assert_nil result
129134
end
135+
136+
test "with_exception_handling returns nil to prevent raw_response pollution" do
137+
# This test verifies the fix for issue #305
138+
# When an exception is handled, with_exception_handling should return nil
139+
# to prevent the handler's return value from becoming raw_response
140+
handler_return_value = "some truthy value that would cause issues"
141+
provider = MockProvider.new(exception_handler: ->(_) { handler_return_value })
142+
provider.error_to_raise = TestError.new("api error")
143+
144+
result = provider.perform_operation
145+
146+
# Even though the handler returns a truthy value, with_exception_handling
147+
# should discard it and return nil to avoid polluting raw_response
148+
assert_nil result, "with_exception_handling should return nil after handling an exception"
149+
end
130150
end
131151
end
132152
end

0 commit comments

Comments
 (0)