Ruby SDK: All tests passing (412/412 unit, 137/138 cross-SDK)#39
Ruby SDK: All tests passing (412/412 unit, 137/138 cross-SDK)#39
Conversation
- Add custom_field_value method for accessing custom field values - Fix type coercion for custom field values (json, number, boolean types)
- Upgrade Ruby version from 3.0.6 to 3.3.10 - Update gem dependencies for compatibility - Add peek alias to peek_treatment method
- Add object key containment test for in_operator - Add peek_treatment audience re-evaluation tests - Add treatment audience re-evaluation tests for strict/non-strict modes - Add tests for audience cache invalidation behavior
- Add publish behavior tests - Add refresh error handling tests - Add HTTP retry logic tests - Add error recovery and resilience tests - Add concurrent operations tests - Add attribute timestamp validation tests - Add variable override precedence tests Total: 55 new tests added, all 316 tests pass
Add 36 Murmur3 parameterized tests and ~36 context tests covering cache invalidation, treatment/exposure queuing, publish data, finalize, unit management, refresh logging, variable value edge cases, track validation, and event logging.
- Fix frozen string literal issues across codebase - Add explicit ostruct require for Ruby 3.3+ compatibility - Fix audience matcher null handling - Fix match operator regex evaluation - Fix context event exposure deduplication - Add type_utils module for consistent type handling - Update experiment model with custom field support - Fix variable parser edge cases
WalkthroughThis pull request introduces substantial enhancements to the Ruby SDK's initialization, context creation, and error handling infrastructure. The ABSmartly class is refactored to support both configuration-object and named-parameter initialization patterns. A new asynchronous context creation API (create_context_async) is added alongside improved thread-safety primitives in the Context class. Error handling is refined across multiple deserializers and operators with targeted exception handling and logging. A new TypeUtils module consolidates utility methods for type conversions and string comparisons. Backwards-compatibility aliases (SDK and SDKConfig) are introduced. Ruby version is updated to 3.3.10. Comprehensive test coverage is added for asynchronous contexts, concurrency, and backwards compatibility, whilst documentation in the README is significantly expanded. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Fix EqualsOperator for numeric string comparisons and InOperator for string containment. Add named params and backwards compatibility specs.
Add create_context_async for non-blocking context creation, plus updated README with async examples.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/json/experiment.rb (1)
30-39:⚠️ Potential issue | 🟠 MajorUse the normalised key for conversion branches.
ALLOWED_KEYSaccepts multiple key forms, but Line 30/Line 32/Line 34 branch on rawname. Keys like"applications"or:custom_field_valuescan bypass class conversion and leave raw hashes in instance state.💡 Proposed fix
- if name == :applications + if key_str == "applications" `@applications` = assign_to_klass(ExperimentApplication, value) - elsif name == :variants + elsif key_str == "variants" `@variants` = assign_to_klass(ExperimentVariant, value) - elsif name == :customFieldValues - if value != nil - `@custom_field_values` = assign_to_klass(CustomFieldValue, value) - end + elsif key_str == "custom_field_values" + `@custom_field_values` = assign_to_klass(CustomFieldValue, value) unless value.nil? else self.instance_variable_set("@#{key_str}", value) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/json/experiment.rb` around lines 30 - 39, The conversion branches are using the raw name variable so keys like "applications" or :custom_field_values bypass class conversion; modify the branch conditions to check the normalized key used by ALLOWED_KEYS (e.g., use key_str or the normalized symbol instead of name) so that assign_to_klass(ExperimentApplication, value), assign_to_klass(ExperimentVariant, value) and assign_to_klass(CustomFieldValue, value) run for all equivalent key forms; update the if/elsif/elsif to compare the normalized key (the same normalization ALLOWED_KEYS expects) before setting `@applications`, `@variants`, or `@custom_field_values` and keep the fallback instance_variable_set as-is.
🧹 Nitpick comments (9)
lib/context_event_logger_callback.rb (1)
11-11: Redundant nil check.The condition
@callable && !@callable.nil?is redundant — if@callableis truthy, it is inherently notnil. This can be simplified.✨ Suggested simplification
- `@callable.call`(event, data) if `@callable` && !@callable.nil? + `@callable.call`(event, data) if `@callable`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/context_event_logger_callback.rb` at line 11, Replace the redundant nil-check in the call site: instead of "@callable.call(event, data) if `@callable` && !@callable.nil?" simplify to either "if `@callable`" or use the safe navigation operator "@callable&.call(event, data)". Update the line where `@callable` is invoked so only one truthiness check is performed (reference: the "@callable.call(event, data)" invocation and the "@callable" instance variable).lib/json_expr/operators/match_operator.rb (1)
23-33: Prefer regexp-level timeout when available, with fallback for older Ruby.Using
Timeout.timeoutaround regex matching raises asynchronous exceptions unpredictably. Regexp-native timeout, available in Ruby 3.2+, is safer. Since this repository supports Ruby 2.7, the fallback toTimeout.timeoutremains necessary.The proposed refactor below uses runtime detection for compatibility, though caching the check result would avoid repeated inspection:
♻️ Proposed compatibility-safe refactor
- begin - Timeout.timeout(MATCH_TIMEOUT) do - Regexp.new(pattern).match(text) - end + begin + supports_regexp_timeout = Regexp.method(:new).parameters.any? { |_, name| name == :timeout } + + if supports_regexp_timeout + Regexp.new(pattern, timeout: MATCH_TIMEOUT).match(text) + else + Timeout.timeout(MATCH_TIMEOUT) { Regexp.new(pattern).match(text) } + end rescue Timeout::Error warn("Regex match timeout: pattern=#{pattern[0..50].inspect}...") nillib/context.rb (1)
632-632: Minor: unnecessary||=in initialisation.
@exposures ||= []uses conditional assignment, but since this is fresh initialisation, a simple assignment@exposures = []would be clearer and consistent with the other initialisations in this method.Proposed fix
- `@exposures` ||= [] + `@exposures` = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/context.rb` at line 632, Replace the conditional assignment for the instance variable `@exposures` with a direct assignment to match the other initialisations in the same method: change the initialization from using ||= to a simple = assignment so `@exposures` is explicitly set to an empty array during context setup (look for the line setting `@exposures` and the surrounding initialisation block in the same method).spec/http_retry_spec.rb (1)
8-15: Remove the unusedclientsetup in this example.Line 12 currently adds setup noise without contributing to the assertion.
💡 Proposed fix
it "configures max_retries correctly" do config = DefaultHttpClientConfig.create config.max_retries = 3 - - client = DefaultHttpClient.create(config) expect(config.max_retries).to eq(3) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/http_retry_spec.rb` around lines 8 - 15, The test creates an unused client variable which adds noise; remove the unnecessary call to DefaultHttpClient.create(config) (the line assigning to client) from the example so the spec only sets config via DefaultHttpClientConfig.create and asserts config.max_retries equals 3; references: DefaultHttpClient.create, DefaultHttpClientConfig.create, and the local variable client.spec/concurrency_spec.rb (1)
67-72: Trim redundantbeginwrappers in thread blocks.These
begin ... rescuewrappers are unnecessary in block scope and add noise; you can keep the same rescue behaviour with less nesting.Also applies to: 113-117, 161-170, 206-210, 231-236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/concurrency_spec.rb` around lines 67 - 72, Remove the redundant begin...end wrappers inside the thread blocks that call context.treatment("exp_test_ab"); instead write the rescue directly on the block (thread do ... rescue StandardError => e ... end) so behaviour is identical but nesting is reduced—keep mutex.synchronize { results << result } and mutex.synchronize { errors << e } unchanged; apply the same change for the other thread blocks that use context.treatment, mutex, results and errors in this spec.spec/context_spec.rb (1)
1364-1364: Remove unused localoriginal_experiments.Line 1364 assigns
original_experimentsbut never uses it.♻️ Tiny cleanup
- original_experiments = context.experiments.dup - context.refresh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/context_spec.rb` at line 1364, Remove the unused local variable assignment by deleting the line that sets original_experiments = context.experiments.dup; locate the assignment by the symbol original_experiments and the call to context.experiments.dup in spec/context_spec.rb and remove it to clean up the unused local without changing any other behavior.README.md (1)
14-14: Consider removing shell prompts in command blocks.Line 14 and Line 20 use
$prefixes, which triggersMD014and is usually avoided in copy/paste snippets.Also applies to: 20-20
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 14, Remove the leading shell prompt from the command examples so they are plain copy/paste-friendly; replace occurrences of "$ bundle add absmartly-sdk" and the other "$ ..." command in the README with the command text without the "$" prefix (e.g., "bundle add absmartly-sdk") in the code blocks to resolve MD014 and improve usability.spec/a_b_smartly_named_params_spec.rb (1)
37-57: Default timeout/retries tests do not assert the defaults they name.Line 37 and Line 48 currently pass even if defaults regress, because they only check
sdkis non-nil.💡 Suggested test tightening
it "uses default timeout of 3000ms" do + captured_config = nil + allow(Client).to receive(:create) do |cfg, *_| + captured_config = cfg + instance_double(Client) + end + sdk = ABSmartly.new( valid_params[:endpoint], api_key: valid_params[:api_key], application: valid_params[:application], environment: valid_params[:environment] ) expect(sdk).not_to be_nil + expect(captured_config.connect_timeout).to eq(3.0) + expect(captured_config.connection_request_timeout).to eq(3.0) end it "uses default retries of 5" do + captured_config = nil + allow(Client).to receive(:create) do |cfg, *_| + captured_config = cfg + instance_double(Client) + end + sdk = ABSmartly.new( valid_params[:endpoint], api_key: valid_params[:api_key], application: valid_params[:application], environment: valid_params[:environment] ) expect(sdk).not_to be_nil + expect(captured_config.max_retries).to eq(5) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/a_b_smartly_named_params_spec.rb` around lines 37 - 57, The tests currently only assert sdk is non-nil but should verify the actual default values; update the two examples that call ABSmartly.new to replace the expect(sdk).not_to be_nil with assertions that the instance uses the expected defaults (e.g. expect(sdk.options[:timeout]).to eq(3000) for the timeout test and expect(sdk.options[:retries]).to eq(5) for the retries test), locating the instance created via ABSmartly.new (variable sdk) and adjusting the accessor (options, config or accessor methods like timeout/retries) to match your SDK internals.spec/create_context_async_spec.rb (1)
130-143: The pre-ready test leaves a long-running worker thread behind.Line 132 uses a 5-second delay and the example exits without draining that async fetch. This can slow or destabilise the suite.
💡 Suggested adjustment
- config.client = slow_client_mock(delay: 5) + config.client = slow_client_mock(delay: 0.2) @@ expect { context.treatment("exp_test_ab") }.to raise_error( IllegalStateException, "ABSmartly Context is not yet ready" ) + + # Drain async worker for test hygiene + context.wait_until_ready(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/create_context_async_spec.rb` around lines 130 - 143, The test leaves a long-running worker from the slow_client_mock( delay: 5 ) behind; change the mock to use a tiny delay (e.g. delay: 0.01) or replace with a non-blocking/faster stub so create_context_async (via ABSmartly.create and sdk.create_context_async) doesn't spawn a long-lived fetch, and/or ensure the background worker is drained by calling the SDK shutdown/cleanup (e.g. sdk.shutdown or equivalent) after the expectation so the test process doesn't retain the async thread.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/a_b_smartly.rb`:
- Around line 89-93: Replace the permissive .to_i/.to_f coercions with strict
Integer()/Float() conversions for parameters so invalid strings raise
immediately: validate timeout using Float(timeout) and ensure it is > 0 (raise
ArgumentError with a clear message on failure), validate retries using
Integer(retries) and ensure it is >= 0 (raise ArgumentError on failure), and
when assigning to client_config.max_retries use Integer(retries) (and similarly
use Float(timeout) where the numeric timeout is forwarded) so only real numeric
values are accepted and propagated to Faraday.
- Around line 127-130: The background thread that calls
data_provider.context_data and then context.set_data must rescue exceptions so
wait_until_ready() isn't left blocked; wrap the Thread.new body in a
begin/rescue, catch any exception from data_provider.context_data,
construct/passthrough a failure object (or the exception) to context.set_data so
that context.set_data_failed runs and `@ready_condvar` is broadcast, and re-use
the existing symbols: Thread.new, data_provider.context_data, context.set_data,
set_data_failed, wait_until_ready, and `@ready_condvar`.
In `@lib/audience_matcher.rb`:
- Around line 38-39: The rescue handler in audience_matcher.rb can raise if
e.backtrace is nil; update the warn call that logs the exception (the rescue
block using "rescue StandardError => e" and the warn("Audience evaluation
failed: #{e.class} - #{e.message}\n#{e.backtrace.first(5).join("\n")}")
invocation) to normalize the backtrace with Array(e.backtrace) before calling
first(5).join so the log is nil-safe and won't raise a NoMethodError.
In `@lib/client.rb`:
- Around line 55-57: Replace raw response-body usage in the error path: instead
of creating Exception.new(`@promise.body`) and logging warn("Failed to fetch
context data: #{`@promise.body`}"), build the exception and log using
non-sensitive metadata (e.g. `@promise.status`, `@promise.code`, or `@promise.reason`)
and a sanitized/truncated body snippet if required (e.g. first N chars with
sensitive patterns redacted). Update the two locations that set `@exception` and
call warn (the block using `@promise` shown and the similar block around lines
69-72) to use Exception.new with a safe message and warn with status/code plus a
short, redacted/truncated body preview rather than the full `@promise.body`.
In `@lib/context.rb`:
- Line 610: The `@index` instance variable is initialized as an Array but used as
a Hash elsewhere (e.g., assign_data and accesses like `@index`[experiment.name]);
change the initialization from [] to {} so `@index` is consistently a Hash,
ensuring code paths guarded by check_ready? continue to work with the correct
type (update the declaration of `@index` to use {}).
In `@lib/default_audience_deserializer.rb`:
- Line 9: Replace the inclusive-range slice used in the JSON.parse call
(currently JSON.parse(bytes[offset..length], ...)) with the start/length form to
match the other deserializer; i.e., change the slice to bytes[offset, length] so
both deserializers use the same semantics when parsing the JSON payload.
In `@lib/json/experiment.rb`:
- Around line 47-52: The method assign_to_klass uses a method-level `return`
inside arr.map which prematurely exits the method; replace the `return item if
item.is_a?(klass)` with a non-return expression (e.g., yield the item back to
the map by using `item` or `next item`) so the block always produces values for
mapping, keep the `klass.new(*item.values)` branch as-is, and ensure the final
`.compact` call still returns an array of instances; reference arr.map,
item.is_a?(klass), klass.new(*item.values), and assign_to_klass when making the
change.
In `@README.md`:
- Around line 351-366: The example CustomEventLogger#handle_event assumes event
payloads are Hashes (using data[:...]) but the SDK provides typed objects (e.g.,
Exposure, GoalAchievement, ContextData), which will raise NoMethodError; update
the example to access attributes on the objects (e.g., use data.name or
data.experiments instead of data[:name]/data[:experiments], and call length on
the returned array where needed) for the cases handling EVENT_TYPE::EXPOSURE,
EVENT_TYPE::GOAL, EVENT_TYPE::PUBLISH, EVENT_TYPE::READY, and
EVENT_TYPE::REFRESH so the code uses object attribute access consistent with the
payload types.
---
Outside diff comments:
In `@lib/json/experiment.rb`:
- Around line 30-39: The conversion branches are using the raw name variable so
keys like "applications" or :custom_field_values bypass class conversion; modify
the branch conditions to check the normalized key used by ALLOWED_KEYS (e.g.,
use key_str or the normalized symbol instead of name) so that
assign_to_klass(ExperimentApplication, value),
assign_to_klass(ExperimentVariant, value) and assign_to_klass(CustomFieldValue,
value) run for all equivalent key forms; update the if/elsif/elsif to compare
the normalized key (the same normalization ALLOWED_KEYS expects) before setting
`@applications`, `@variants`, or `@custom_field_values` and keep the fallback
instance_variable_set as-is.
---
Nitpick comments:
In `@lib/context_event_logger_callback.rb`:
- Line 11: Replace the redundant nil-check in the call site: instead of
"@callable.call(event, data) if `@callable` && !@callable.nil?" simplify to either
"if `@callable`" or use the safe navigation operator "@callable&.call(event,
data)". Update the line where `@callable` is invoked so only one truthiness check
is performed (reference: the "@callable.call(event, data)" invocation and the
"@callable" instance variable).
In `@lib/context.rb`:
- Line 632: Replace the conditional assignment for the instance variable
`@exposures` with a direct assignment to match the other initialisations in the
same method: change the initialization from using ||= to a simple = assignment
so `@exposures` is explicitly set to an empty array during context setup (look for
the line setting `@exposures` and the surrounding initialisation block in the same
method).
In `@README.md`:
- Line 14: Remove the leading shell prompt from the command examples so they are
plain copy/paste-friendly; replace occurrences of "$ bundle add absmartly-sdk"
and the other "$ ..." command in the README with the command text without the
"$" prefix (e.g., "bundle add absmartly-sdk") in the code blocks to resolve
MD014 and improve usability.
In `@spec/a_b_smartly_named_params_spec.rb`:
- Around line 37-57: The tests currently only assert sdk is non-nil but should
verify the actual default values; update the two examples that call
ABSmartly.new to replace the expect(sdk).not_to be_nil with assertions that the
instance uses the expected defaults (e.g. expect(sdk.options[:timeout]).to
eq(3000) for the timeout test and expect(sdk.options[:retries]).to eq(5) for the
retries test), locating the instance created via ABSmartly.new (variable sdk)
and adjusting the accessor (options, config or accessor methods like
timeout/retries) to match your SDK internals.
In `@spec/concurrency_spec.rb`:
- Around line 67-72: Remove the redundant begin...end wrappers inside the thread
blocks that call context.treatment("exp_test_ab"); instead write the rescue
directly on the block (thread do ... rescue StandardError => e ... end) so
behaviour is identical but nesting is reduced—keep mutex.synchronize { results
<< result } and mutex.synchronize { errors << e } unchanged; apply the same
change for the other thread blocks that use context.treatment, mutex, results
and errors in this spec.
In `@spec/context_spec.rb`:
- Line 1364: Remove the unused local variable assignment by deleting the line
that sets original_experiments = context.experiments.dup; locate the assignment
by the symbol original_experiments and the call to context.experiments.dup in
spec/context_spec.rb and remove it to clean up the unused local without changing
any other behavior.
In `@spec/create_context_async_spec.rb`:
- Around line 130-143: The test leaves a long-running worker from the
slow_client_mock( delay: 5 ) behind; change the mock to use a tiny delay (e.g.
delay: 0.01) or replace with a non-blocking/faster stub so create_context_async
(via ABSmartly.create and sdk.create_context_async) doesn't spawn a long-lived
fetch, and/or ensure the background worker is drained by calling the SDK
shutdown/cleanup (e.g. sdk.shutdown or equivalent) after the expectation so the
test process doesn't retain the async thread.
In `@spec/http_retry_spec.rb`:
- Around line 8-15: The test creates an unused client variable which adds noise;
remove the unnecessary call to DefaultHttpClient.create(config) (the line
assigning to client) from the example so the spec only sets config via
DefaultHttpClientConfig.create and asserts config.max_retries equals 3;
references: DefaultHttpClient.create, DefaultHttpClientConfig.create, and the
local variable client.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
.gitignore.ruby-versionCHANGELOG.mdREADME.mdabsmartly.gemspecexample/example.rblib/a_b_smartly.rblib/a_b_smartly_config.rblib/absmartly.rblib/audience_matcher.rblib/client.rblib/context.rblib/context_event_logger_callback.rblib/default_audience_deserializer.rblib/default_context_data_deserializer.rblib/default_variable_parser.rblib/json/experiment.rblib/json_expr/expr_evaluator.rblib/json_expr/operators/and_combinator.rblib/json_expr/operators/equals_operator.rblib/json_expr/operators/in_operator.rblib/json_expr/operators/match_operator.rblib/json_expr/operators/or_combinator.rblib/scheduled_thread_pool_executor.rblib/type_utils.rbspec/a_b_smartly_named_params_spec.rbspec/backwards_compatibility_spec.rbspec/concurrency_spec.rbspec/context_spec.rbspec/create_context_async_spec.rbspec/default_variable_parser_spec.rbspec/hashing_spec.rbspec/http_retry_spec.rbspec/json_expr/operators/equals_operator_spec.rbspec/json_expr/operators/in_operator_spec.rb
| retries ||= 5 | ||
|
|
||
| raise ArgumentError.new("timeout must be a positive number") if timeout.to_i <= 0 | ||
| raise ArgumentError.new("retries must be a non-negative number") if retries.to_i < 0 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lib/a_b_smartly.rb | sed -n '85,110p'Repository: absmartly/ruby-sdk
Length of output: 1468
🏁 Script executed:
fd -t f -n "client_config\|ClientConfig" --ignore-case | head -20Repository: absmartly/ruby-sdk
Length of output: 292
🏁 Script executed:
fd -t f "client_config|ClientConfig" --ignore-case | head -20Repository: absmartly/ruby-sdk
Length of output: 183
🏁 Script executed:
cat -n lib/client_config.rbRepository: absmartly/ruby-sdk
Length of output: 2158
🏁 Script executed:
cat -n lib/default_http_client_config.rbRepository: absmartly/ruby-sdk
Length of output: 732
🏁 Script executed:
rg "max_retries" -A 3 -B 1Repository: absmartly/ruby-sdk
Length of output: 6460
🏁 Script executed:
rg "request :retry" -B 5 -A 5 lib/default_http_client.rbRepository: absmartly/ruby-sdk
Length of output: 391
🏁 Script executed:
cat -n spec/http_retry_spec.rbRepository: absmartly/ruby-sdk
Length of output: 6285
🏁 Script executed:
rg "retries.*abc|to_i|Integer|Float" -B 2 -A 2 spec/Repository: absmartly/ruby-sdk
Length of output: 1117
🏁 Script executed:
cat -n lib/a_b_smartly.rb | sed -n '1,30p'Repository: absmartly/ruby-sdk
Length of output: 1222
🏁 Script executed:
rg "ArgumentError.*retries|ArgumentError.*timeout" -B 3 -A 3 spec/Repository: absmartly/ruby-sdk
Length of output: 1453
🏁 Script executed:
cat -n spec/a_b_smartly_named_params_spec.rb | sed -n '1,100p'Repository: absmartly/ruby-sdk
Length of output: 3755
🏁 Script executed:
cat -n spec/a_b_smartly_named_params_spec.rb | sed -n '98,130p'Repository: absmartly/ruby-sdk
Length of output: 1295
Use Integer() and Float() for strict parameter coercion instead of .to_i and .to_f.
The validation on line 92 uses .to_i < 0, which allows non-numeric strings like "abc" to pass because "abc".to_i returns 0. The raw value is then forwarded to client_config.max_retries on line 101, where it eventually reaches Faraday's retry middleware expecting a numeric value, causing a runtime error.
Use Ruby's strict conversion methods (Integer() and Float()) to reject invalid input at initialization time:
💡 Proposed fix
- timeout ||= 3000
- retries ||= 5
-
- raise ArgumentError.new("timeout must be a positive number") if timeout.to_i <= 0
- raise ArgumentError.new("retries must be a non-negative number") if retries.to_i < 0
+ timeout = 3000 if timeout.nil?
+ retries = 5 if retries.nil?
+
+ begin
+ timeout = Float(timeout)
+ rescue ArgumentError, TypeError
+ raise ArgumentError.new("timeout must be a positive number")
+ end
+
+ begin
+ retries = Integer(retries)
+ rescue ArgumentError, TypeError
+ raise ArgumentError.new("retries must be a non-negative number")
+ end
+
+ raise ArgumentError.new("timeout must be a positive number") if timeout <= 0
+ raise ArgumentError.new("retries must be a non-negative number") if retries < 0
@@
- client_config.connect_timeout = timeout.to_f / 1000.0
- client_config.connection_request_timeout = timeout.to_f / 1000.0
+ client_config.connect_timeout = timeout / 1000.0
+ client_config.connection_request_timeout = timeout / 1000.0
client_config.max_retries = retriesAlso applies to: 101-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/a_b_smartly.rb` around lines 89 - 93, Replace the permissive .to_i/.to_f
coercions with strict Integer()/Float() conversions for parameters so invalid
strings raise immediately: validate timeout using Float(timeout) and ensure it
is > 0 (raise ArgumentError with a clear message on failure), validate retries
using Integer(retries) and ensure it is >= 0 (raise ArgumentError on failure),
and when assigning to client_config.max_retries use Integer(retries) (and
similarly use Float(timeout) where the numeric timeout is forwarded) so only
real numeric values are accepted and propagated to Faraday.
| Thread.new do | ||
| data_future = data_provider.context_data | ||
| context.set_data(data_future) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file at the specified lines
cat -n lib/a_b_smartly.rb | sed -n '120,140p'Repository: absmartly/ruby-sdk
Length of output: 1002
🏁 Script executed:
# Get file size to understand scope
wc -l lib/a_b_smartly.rbRepository: absmartly/ruby-sdk
Length of output: 84
🏁 Script executed:
# Search for context.set_data definition
rg "def set_data" -A 5 -B 2Repository: absmartly/ruby-sdk
Length of output: 711
🏁 Script executed:
# Look for context waiting/blocking mechanisms
rg "wait|ready|join" -B 2 -A 2 -t rbRepository: absmartly/ruby-sdk
Length of output: 88
🏁 Script executed:
# Check if there's existing exception handling in data_provider context calls
rg "data_provider\.context_data" -B 5 -A 5Repository: absmartly/ruby-sdk
Length of output: 6811
🏁 Script executed:
# Look for similar thread patterns to understand established conventions
rg "Thread\.new" -B 3 -A 5 -t rbRepository: absmartly/ruby-sdk
Length of output: 88
🏁 Script executed:
# Get full set_data and set_data_failed implementations
cat -n lib/context.rb | grep -A 15 "def set_data"Repository: absmartly/ruby-sdk
Length of output: 1120
🏁 Script executed:
# Find wait_until_ready implementation
rg "def wait_until_ready" -A 10Repository: absmartly/ruby-sdk
Length of output: 584
🏁 Script executed:
# Look at context_data provider to understand what it returns
rg "def context_data" -A 10 -B 2Repository: absmartly/ruby-sdk
Length of output: 4087
🏁 Script executed:
# Check the async creation tests to see expected behavior on exceptions
rg "create_context_async" -B 3 -A 10 spec/Repository: absmartly/ruby-sdk
Length of output: 8440
🏁 Script executed:
# Check if ostruct is already required in the file
rg "require.*ostruct" lib/Repository: absmartly/ruby-sdk
Length of output: 44
🏁 Script executed:
# Check what's required at the top of lib/a_b_smartly.rb
head -20 lib/a_b_smartly.rbRepository: absmartly/ruby-sdk
Length of output: 720
🏁 Script executed:
# Verify if OpenStruct is used elsewhere in the codebase
rg "OpenStruct" -B 2 -A 2Repository: absmartly/ruby-sdk
Length of output: 6663
Add exception handling to background data fetch to prevent indefinite blocking on wait_until_ready().
If data_provider.context_data raises an exception in the background thread, context.set_data is never called and @ready_condvar is never broadcast, causing any caller to wait_until_ready() to block indefinitely.
Wrap the call in a begin/rescue block and pass a failure object to context.set_data so that set_data_failed is invoked and the condition variable is properly signalled.
Proposed fix
+require "ostruct"
+
Thread.new do
- data_future = data_provider.context_data
- context.set_data(data_future)
+ begin
+ data_future = data_provider.context_data
+ context.set_data(data_future)
+ rescue StandardError => e
+ context.set_data(OpenStruct.new(exception: e, success?: false, data_future: nil))
+ end
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Thread.new do | |
| data_future = data_provider.context_data | |
| context.set_data(data_future) | |
| end | |
| Thread.new do | |
| begin | |
| data_future = data_provider.context_data | |
| context.set_data(data_future) | |
| rescue StandardError => e | |
| context.set_data(OpenStruct.new(exception: e, success?: false, data_future: nil)) | |
| end | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/a_b_smartly.rb` around lines 127 - 130, The background thread that calls
data_provider.context_data and then context.set_data must rescue exceptions so
wait_until_ready() isn't left blocked; wrap the Thread.new body in a
begin/rescue, catch any exception from data_provider.context_data,
construct/passthrough a failure object (or the exception) to context.set_data so
that context.set_data_failed runs and `@ready_condvar` is broadcast, and re-use
the existing symbols: Thread.new, data_provider.context_data, context.set_data,
set_data_failed, wait_until_ready, and `@ready_condvar`.
| rescue StandardError => e | ||
| warn("Audience evaluation failed: #{e.class} - #{e.message}\n#{e.backtrace.first(5).join("\n")}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate and examine the file
fd -t f "audience_matcher.rb" | head -5Repository: absmartly/ruby-sdk
Length of output: 85
🌐 Web query:
Ruby 3.3 Exception#backtrace nil safe handling Array wrapper pattern
💡 Result:
In Ruby 3.3, Exception#backtrace legitimately returns Array(String) or nil: if no backtrace has been set (e.g., StandardError.new.backtrace), Ruby returns nil. [1]
Nil-safe “Array wrapper” pattern
Use Array(...) to normalize to an array:
bt = Array(e.backtrace) # => [] if nil, else the array
bt.each { |line| warn line }This works because Array(nil) #=> [], while Array(existing_array) #=> existing_array.
Common alternatives
e.backtrace&.join("\n") # nil-safe join
(e.backtrace || []).each { ... } # explicit fallbackRelated: backtrace_locations can also be nil
Exception#backtrace_locations similarly returns an array or nil, depending on what backtrace data exists / how it was set. [2]
When/why it might be nil
A backtrace is normally set when an exception is raised, but you can also construct exceptions without raising them, or explicitly set/override backtraces (including via raise(..., backtrace_array)), which affects what #backtrace / #backtrace_locations return. [1][3]
Sources: Ruby 3.3 Exception#backtrace docs [1], Ruby backtrace behavior notes [2], Ruby Kernel#raise backtrace argument docs [3].
🏁 Script executed:
cat -n lib/audience_matcher.rb | sed -n '30,45p'Repository: absmartly/ruby-sdk
Length of output: 575
Guard against nil backtraces in error logging.
Line 39 calls e.backtrace.first(5) without nil-safety; in Ruby 3.3, Exception#backtrace can return nil (e.g. when an exception is constructed without being raised), which causes NoMethodError within the rescue handler itself. Use Array(e.backtrace) to safely normalise to an empty array if nil:
Proposed fix
rescue StandardError => e
- warn("Audience evaluation failed: #{e.class} - #{e.message}\n#{e.backtrace.first(5).join("\n")}")
+ backtrace = Array(e.backtrace).first(5).join("\n")
+ suffix = backtrace.empty? ? "" : "\n#{backtrace}"
+ warn("Audience evaluation failed: #{e.class} - #{e.message}#{suffix}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rescue StandardError => e | |
| warn("Audience evaluation failed: #{e.class} - #{e.message}\n#{e.backtrace.first(5).join("\n")}") | |
| rescue StandardError => e | |
| backtrace = Array(e.backtrace).first(5).join("\n") | |
| suffix = backtrace.empty? ? "" : "\n#{backtrace}" | |
| warn("Audience evaluation failed: #{e.class} - #{e.message}#{suffix}") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/audience_matcher.rb` around lines 38 - 39, The rescue handler in
audience_matcher.rb can raise if e.backtrace is nil; update the warn call that
logs the exception (the rescue block using "rescue StandardError => e" and the
warn("Audience evaluation failed: #{e.class} -
#{e.message}\n#{e.backtrace.first(5).join("\n")}") invocation) to normalize the
backtrace with Array(e.backtrace) before calling first(5).join so the log is
nil-safe and won't raise a NoMethodError.
| @exception = Exception.new(@promise.body) | ||
| warn("Failed to fetch context data: #{@promise.body}") | ||
| return self |
There was a problem hiding this comment.
Do not log raw response bodies in warning paths.
These warnings can leak sensitive data into logs. Prefer status/error metadata (or a sanitised/truncated message) instead of full body text.
💡 Proposed fix
unless `@promise.success`?
`@exception` = Exception.new(`@promise.body`)
- warn("Failed to fetch context data: #{`@promise.body`}")
+ warn("Failed to fetch context data (status=#{`@promise.respond_to`?(:status) ? `@promise.status` : 'unknown'})")
return self
end
@@
unless response.success?
error = Exception.new(response.body)
- warn("Publish failed: #{response.body}")
+ warn("Publish failed (status=#{response.respond_to?(:status) ? response.status : 'unknown'})")
return error
endAlso applies to: 69-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/client.rb` around lines 55 - 57, Replace raw response-body usage in the
error path: instead of creating Exception.new(`@promise.body`) and logging
warn("Failed to fetch context data: #{`@promise.body`}"), build the exception and
log using non-sensitive metadata (e.g. `@promise.status`, `@promise.code`, or
`@promise.reason`) and a sanitized/truncated body snippet if required (e.g. first
N chars with sensitive patterns redacted). Update the two locations that set
`@exception` and call warn (the block using `@promise` shown and the similar block
around lines 69-72) to use Exception.new with a safe message and warn with
status/code plus a short, redacted/truncated body preview rather than the full
`@promise.body`.
|
|
||
| def init_common(clock, config, data_provider, event_handler, event_logger, | ||
| variable_parser, audience_matcher) | ||
| @index = [] |
There was a problem hiding this comment.
Type inconsistency: @index initialised as Array but used as Hash.
@index is initialised as [] here, but assign_data (line 533) sets @index = {} and subsequent code accesses it as a Hash (e.g., @index[experiment.name] on line 582). Whilst check_ready? guards most accesses, initialising as an empty Hash would be more consistent.
Proposed fix
- `@index` = []
+ `@index` = {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @index = [] | |
| `@index` = {} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/context.rb` at line 610, The `@index` instance variable is initialized as
an Array but used as a Hash elsewhere (e.g., assign_data and accesses like
`@index`[experiment.name]); change the initialization from [] to {} so `@index` is
consistently a Hash, ensuring code paths guarded by check_ready? continue to
work with the correct type (update the declaration of `@index` to use {}).
| @@ -7,7 +7,11 @@ class DefaultAudienceDeserializer < AudienceDeserializer | |||
|
|
|||
| def deserialize(bytes, offset, length) | |||
| JSON.parse(bytes[offset..length], symbolize_names: true) | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the default_audience_deserializer.rb file around line 9
echo "=== default_audience_deserializer.rb ==="
cat -n lib/default_audience_deserializer.rb
echo ""
echo "=== default_context_data_deserializer.rb ==="
cat -n lib/default_context_data_deserializer.rbRepository: absmartly/ruby-sdk
Length of output: 1516
🏁 Script executed:
#!/bin/bash
# Check if there are other deserializer classes to understand the pattern
echo "=== Finding all deserializer files ==="
fd -t f "deserializer" lib/ | head -20
echo ""
echo "=== Checking other deserializer implementations ==="
for file in $(fd -t f "deserializer" lib/ | grep -i default); do
echo "=== $file ==="
grep -n "bytes\[" "$file" || echo "(no bytes slicing found)"
doneRepository: absmartly/ruby-sdk
Length of output: 522
Inconsistent string slicing between deserializers.
The two deserializers use different slicing approaches with identical method signatures. lib/default_audience_deserializer.rb line 9 uses bytes[offset..length] (inclusive range from offset to length), whilst lib/default_context_data_deserializer.rb line 11 uses bytes[offset, length] (slice: length bytes starting at offset). These have different semantics:
bytes[0..10]returns 11 characters (indices 0–10 inclusive)bytes[0, 10]returns 10 characters starting at index 0
Since both methods have identical signatures and receive the same offset and length parameters, they should likely behave consistently. Align the slicing approach across both deserializers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/default_audience_deserializer.rb` at line 9, Replace the inclusive-range
slice used in the JSON.parse call (currently JSON.parse(bytes[offset..length],
...)) with the start/length form to match the other deserializer; i.e., change
the slice to bytes[offset, length] so both deserializers use the same semantics
when parsing the JSON payload.
| arr.map do |item| | ||
| next if item.nil? | ||
| return item if item.is_a?(klass) | ||
|
|
||
| klass.new(*item.values) | ||
| end | ||
| end.compact |
There was a problem hiding this comment.
Avoid method-level return inside map.
At Line 49, return item exits assign_to_klass early and can return a single instance instead of an array.
💡 Proposed fix
def assign_to_klass(klass, arr)
arr.map do |item|
next if item.nil?
- return item if item.is_a?(klass)
+ next item if item.is_a?(klass)
klass.new(*item.values)
end.compact
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/json/experiment.rb` around lines 47 - 52, The method assign_to_klass uses
a method-level `return` inside arr.map which prematurely exits the method;
replace the `return item if item.is_a?(klass)` with a non-return expression
(e.g., yield the item back to the map by using `item` or `next item`) so the
block always produces values for mapping, keep the `klass.new(*item.values)`
branch as-is, and ensure the final `.compact` call still returns an array of
instances; reference arr.map, item.is_a?(klass), klass.new(*item.values), and
assign_to_klass when making the change.
| class CustomEventLogger < ContextEventLogger | ||
| def handle_event(event, data) | ||
| case event | ||
| when EVENT_TYPE::EXPOSURE | ||
| puts "Exposed to experiment: #{data[:name]}" | ||
| when EVENT_TYPE::GOAL | ||
| puts "Goal tracked: #{data[:name]}" | ||
| when EVENT_TYPE::ERROR | ||
| puts "Error: #{data}" | ||
| when EVENT_TYPE::PUBLISH | ||
| puts "Events published: #{data.length} events" | ||
| when EVENT_TYPE::READY | ||
| puts "Context ready with #{data[:experiments].length} experiments" | ||
| when EVENT_TYPE::REFRESH | ||
| puts "Context refreshed with #{data[:experiments].length} experiments" | ||
| when EVENT_TYPE::CLOSE |
There was a problem hiding this comment.
Custom event logger example uses hash access for object payloads.
On Line 355, Line 357, Line 363, and Line 365, data[:...] assumes Hash payloads, but SDK event payloads here are objects (e.g., Exposure, GoalAchievement, ContextData), so this example can raise NoMethodError.
💡 Proposed doc fix
when EVENT_TYPE::EXPOSURE
- puts "Exposed to experiment: #{data[:name]}"
+ puts "Exposed to experiment: #{data.name}"
when EVENT_TYPE::GOAL
- puts "Goal tracked: #{data[:name]}"
+ puts "Goal tracked: #{data.name}"
@@
when EVENT_TYPE::READY
- puts "Context ready with #{data[:experiments].length} experiments"
+ puts "Context ready with #{data.experiments.length} experiments"
when EVENT_TYPE::REFRESH
- puts "Context refreshed with #{data[:experiments].length} experiments"
+ puts "Context refreshed with #{data.experiments.length} experiments"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class CustomEventLogger < ContextEventLogger | |
| def handle_event(event, data) | |
| case event | |
| when EVENT_TYPE::EXPOSURE | |
| puts "Exposed to experiment: #{data[:name]}" | |
| when EVENT_TYPE::GOAL | |
| puts "Goal tracked: #{data[:name]}" | |
| when EVENT_TYPE::ERROR | |
| puts "Error: #{data}" | |
| when EVENT_TYPE::PUBLISH | |
| puts "Events published: #{data.length} events" | |
| when EVENT_TYPE::READY | |
| puts "Context ready with #{data[:experiments].length} experiments" | |
| when EVENT_TYPE::REFRESH | |
| puts "Context refreshed with #{data[:experiments].length} experiments" | |
| when EVENT_TYPE::CLOSE | |
| class CustomEventLogger < ContextEventLogger | |
| def handle_event(event, data) | |
| case event | |
| when EVENT_TYPE::EXPOSURE | |
| puts "Exposed to experiment: #{data.name}" | |
| when EVENT_TYPE::GOAL | |
| puts "Goal tracked: #{data.name}" | |
| when EVENT_TYPE::ERROR | |
| puts "Error: #{data}" | |
| when EVENT_TYPE::PUBLISH | |
| puts "Events published: #{data.length} events" | |
| when EVENT_TYPE::READY | |
| puts "Context ready with #{data.experiments.length} experiments" | |
| when EVENT_TYPE::REFRESH | |
| puts "Context refreshed with #{data.experiments.length} experiments" | |
| when EVENT_TYPE::CLOSE |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 351 - 366, The example CustomEventLogger#handle_event
assumes event payloads are Hashes (using data[:...]) but the SDK provides typed
objects (e.g., Exposure, GoalAchievement, ContextData), which will raise
NoMethodError; update the example to access attributes on the objects (e.g., use
data.name or data.experiments instead of data[:name]/data[:experiments], and
call length on the returned array where needed) for the cases handling
EVENT_TYPE::EXPOSURE, EVENT_TYPE::GOAL, EVENT_TYPE::PUBLISH, EVENT_TYPE::READY,
and EVENT_TYPE::REFRESH so the code uses object attribute access consistent with
the payload types.
1 participant
Summary
peekalias forpeekTreatmentconsistency with other SDKsTest Results
Key Fixes
require 'ostruct'for Ruby 3.3+ compatibilitytype_utilsmodule for consistent type handlingTest plan
bundle exec rspecSummary by CodeRabbit
Release Notes
New Features
Documentation
Bug Fixes