Repository navigation
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis diff updates project metadata (.ruby-version, .gitignore), expands README and CHANGELOG entries, and adds environment-driven example config. It introduces SDK and SDKConfig aliases, a named-parameter ABSmartly factory, and public create_context_async. Context gains async creation, readiness signalling, attrs_seq tracking and many concurrency-safe helpers. Adds TypeUtils and refactors json_expr operators to use it. Improves error handling across parsers/deserializers, adjusts Client and scheduler internals, introduces ContextPublisher/DefaultContextPublisher, and adds/updates many specs. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2
✨ Finishing Touches
🐰 I hopped through code with eager paws, 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
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
README.md (1)
351-365:⚠️ Potential issue | 🟡 MinorUse object accessors in
CustomEventLogger.
datais documented here as typed event payloads, sodata[:name]anddata[:experiments]will raiseNoMethodErrorin this example. Use attribute accessors instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 351 - 365, The example CustomEventLogger#handle_event uses hash-style access (data[:name], data[:experiments], data.length) which will raise NoMethodError because event payloads expose attributes, not hash keys; update handle_event in CustomEventLogger to use attribute accessors on the payload (e.g., data.name instead of data[:name], data.experiments instead of data[:experiments], use data.events.size or data.experiments.size as appropriate, and for errors use data.message or data.to_s) for the EVENT_TYPE::EXPOSURE, ::GOAL, ::ERROR, ::PUBLISH, ::READY and ::REFRESH branches so you access the typed payload properties correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 13-20: Remove the leading shell prompt from the two install
command snippets in the README: replace "`$ bundle add absmartly-sdk`" and "`$
gem install absmartly-sdk`" code blocks with the plain commands "bundle add
absmartly-sdk" and "gem install absmartly-sdk" so they are copy/paste-friendly
and satisfy markdownlint; update the two fenced code blocks containing those
commands accordingly.
- Around line 516-530: The middleware example's call(env) method may skip
context.close if `@app.call`(env) raises; wrap the call and close in an ensure
block so that context.close is always executed: create context via
Absmartly.create_context(context_config), assign it to env['absmartly.context'],
invoke `@app.call`(env) inside begin/rescue/ensure (or ensure only) and call
context.close in the ensure section to guarantee cleanup and final publish even
on exceptions.
---
Duplicate comments:
In `@README.md`:
- Around line 351-365: The example CustomEventLogger#handle_event uses
hash-style access (data[:name], data[:experiments], data.length) which will
raise NoMethodError because event payloads expose attributes, not hash keys;
update handle_event in CustomEventLogger to use attribute accessors on the
payload (e.g., data.name instead of data[:name], data.experiments instead of
data[:experiments], use data.events.size or data.experiments.size as
appropriate, and for errors use data.message or data.to_s) for the
EVENT_TYPE::EXPOSURE, ::GOAL, ::ERROR, ::PUBLISH, ::READY and ::REFRESH branches
so you access the typed payload properties correctly.
Matches JS SDK behavior where override() and overrides() do not check if the context is finalized. Allows setting overrides after close.
Matches JS SDK behavior where _indexVariables maps each variable key to an array of Experiment objects. Each key in variable_keys now returns [experiment_name, ...] instead of a single string, supporting cases where multiple experiments share a variable key.
Events are only cleared from the queue after a successful publish. On failure, the queue is left intact so events can be retried. Matches JS SDK behavior where pending/exposures/goals are cleared in the .then() success handler, not before the publish call.
…rror, and finalizing? to Context
…_field_type - Add finalize, finalized?, verify finalizing? works - Rename custom_field_type to custom_field_value_type, keep old as alias - Standardize error messages: use 'ABsmartly', add trailing periods, include unit type in unit errors
…ady or closed
Read methods (treatment, peek_treatment, peek, variable_value, peek_variable_value,
variable_keys, experiments, custom_field_keys, custom_field_value, custom_field_value_type)
now return safe defaults (0, default_value, {}, [], nil) and log via event logger
instead of raising exceptions when context is not ready or closed.
Write methods (set_unit, set_attribute, track, publish) continue to raise.
…melCase ivar - Fix #16: Store @index and @index_variables keys as symbols at assignment time, removing per-call transform_keys - Fix #17: Store @Units keys as symbols, enabling consistent lookup regardless of string/symbol input - Fix #20: Rename @experimentCustomFieldValues to @experiment_custom_field_values - Update context_spec to expect symbol key in @Units after set_unit
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/context.rb (2)
301-311:⚠️ Potential issue | 🟠 MajorUnprotected queue state mutations create race condition between
trackandflush.The
trackmethod mutates@pending_countand@achievementswithout synchronisation, whilstflushreads and resets these same fields without synchronisation. Although@queue_mutexexists and is used inqueue_exposure, thetrackandflushmethods bypass it entirely, allowing concurrent calls to lose increments, lose events, or clear freshly queued items.Wrap the queue state mutations in both methods with
@queue_mutex.synchronize:Changes required
In
track(lines 306–308):- `@pending_count` += 1 - `@achievements.push`(achievement) - log_event(ContextEventLogger::EVENT_TYPE::GOAL, achievement) + `@queue_mutex.synchronize` do + `@pending_count` += 1 + `@achievements.push`(achievement) + log_event(ContextEventLogger::EVENT_TYPE::GOAL, achievement) + endIn
flush(lines 362–365 and 368–399):def flush if `@failed` - `@exposures` = [] - `@achievements` = [] - `@pending_count` = 0 + `@queue_mutex.synchronize` do + `@exposures` = [] + `@achievements` = [] + `@pending_count` = 0 + end return `@data_failed` end - if `@pending_count` > 0 - exposures = `@exposures.dup` - achievements = `@achievements.dup` - event_count = `@pending_count` + `@queue_mutex.synchronize` do + return nil unless `@pending_count` > 0 + exposures = `@exposures.dup` + achievements = `@achievements.dup` + event_count = `@pending_count` # ... rest of publishing logic ... + `@exposures` = [] + `@achievements` = [] + `@pending_count` = 0 + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/context.rb` around lines 301 - 311, The track and flush methods mutate and read shared queue state (`@pending_count` and `@achievements`) without locking, creating a race; wrap the mutations and reads in `@queue_mutex.synchronize` to protect them: in method track (the block that increments `@pending_count`, pushes to `@achievements` and calls log_event) enclose those operations in `@queue_mutex.synchronize`, and in method flush (the code that reads/reset `@pending_count` and swaps or clears `@achievements` and any code that constructs the batch for sending) also enclose the read-reset/swap logic in `@queue_mutex.synchronize`; keep existing queue_exposure usage as-is and ensure the synchronized blocks are minimal and only cover the shared-state access.
456-555:⚠️ Potential issue | 🔴 Critical
@assignment_cachelacks thread-safe access; concurrenttreatmentcalls create duplicate exposures.The
assignmentmethod (lines 453–552) reads@assignment_cacheat line 454 and writes at line 550 without synchronisation. When multiple threads calltreatmentfor the same experiment concurrently, both can read a nil cache entry, create separateAssignmentinstances, and store them separately—resulting in duplicate exposures.Add
@assignment_mutexto synchronise cache access:Proposed fix
Initialize the mutex in
init_common(around line 690):`@ready_mutex` = Mutex.new `@ready_condvar` = ConditionVariable.new `@queue_mutex` = Mutex.new + `@assignment_mutex` = Mutex.newWrap cache access in the
assignmentmethod:def assignment(experiment_name) - assignment = `@assignment_cache`[experiment_name.to_s] + `@assignment_mutex.synchronize` do + assignment = `@assignment_cache`[experiment_name.to_s] exp_key = experiment_name.to_s.to_sym if !assignment.nil? # ... existing logic ... end # ... assignment creation logic ... - `@assignment_cache`[experiment_name.to_s] = assignment - assignment + `@assignment_cache`[experiment_name.to_s] = assignment + assignment + end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/context.rb` around lines 456 - 555, The assignment method reads and writes `@assignment_cache` without synchronization causing duplicate Assignment instances under concurrent treatment calls; initialize a Mutex (e.g. `@assignment_mutex`) in init_common and use it to protect cache access in assignment: synchronize to read the cached entry (exp_key), if present return it; if absent compute the Assignment (the logic that builds Assignment.new, sets variant/eligible/etc.), then synchronize again to check the cache a second time and store/return the cached assignment (double-checked locking). Reference the assignment method, `@assignment_cache`, `@overrides`, `@cassignments`, and the Assignment.new creation/assignment code when applying the changes.
♻️ Duplicate comments (1)
lib/a_b_smartly.rb (1)
89-103:⚠️ Potential issue | 🟠 MajorUse strict numeric coercion for
timeout/retriesbefore validation.At Line 92 and Line 93,
.to_iallows invalid values like"abc"to coerce to0, which can pass/warp validation intent and then be propagated (Line 102).💡 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 = retries#!/bin/bash # Verify permissive coercion is still used in constructor validation. cat -n lib/a_b_smartly.rb | sed -n '83,105p' rg -n --type=rb 'timeout\.to_i|retries\.to_i|max_retries\s*=' lib/a_b_smartly.rb🤖 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 - 103, The validation currently uses timeout.to_i and retries.to_i which silently coerce invalid strings to 0; change to strict numeric coercion by parsing timeout and retries with Integer/Float (e.g., parse timeout into a float_ms = Float(timeout) rescue raise ArgumentError and parse retries with Integer(retries) rescue raise ArgumentError), validate the parsed values (timeout_ms > 0, retries >= 0), and then assign the parsed values to client_config.connect_timeout / connection_request_timeout (use parsed_timeout_ms / 1000.0) and client_config.max_retries (use parsed_retries) so only valid numeric inputs reach ClientConfig.create and its attributes.
🧹 Nitpick comments (1)
spec/context_spec.rb (1)
1324-1326: Remove the unusedoriginal_experimentslocal.Line 1325 assigns
original_experimentsbut never reads it; dropping it will keep the spec intent clearer.✂️ Suggested 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` around lines 1324 - 1326, The test assigns an unused local original_experiments after creating the context; remove the entire assignment (the variable original_experiments) so the spec reads only the relevant setup (leave the create_context call and any uses of context.experiments intact), ensuring no other assertions rely on original_experiments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/context.rb`:
- Around line 301-311: The track and flush methods mutate and read shared queue
state (`@pending_count` and `@achievements`) without locking, creating a race; wrap
the mutations and reads in `@queue_mutex.synchronize` to protect them: in method
track (the block that increments `@pending_count`, pushes to `@achievements` and
calls log_event) enclose those operations in `@queue_mutex.synchronize`, and in
method flush (the code that reads/reset `@pending_count` and swaps or clears
`@achievements` and any code that constructs the batch for sending) also enclose
the read-reset/swap logic in `@queue_mutex.synchronize`; keep existing
queue_exposure usage as-is and ensure the synchronized blocks are minimal and
only cover the shared-state access.
- Around line 456-555: The assignment method reads and writes `@assignment_cache`
without synchronization causing duplicate Assignment instances under concurrent
treatment calls; initialize a Mutex (e.g. `@assignment_mutex`) in init_common and
use it to protect cache access in assignment: synchronize to read the cached
entry (exp_key), if present return it; if absent compute the Assignment (the
logic that builds Assignment.new, sets variant/eligible/etc.), then synchronize
again to check the cache a second time and store/return the cached assignment
(double-checked locking). Reference the assignment method, `@assignment_cache`,
`@overrides`, `@cassignments`, and the Assignment.new creation/assignment code when
applying the changes.
---
Duplicate comments:
In `@lib/a_b_smartly.rb`:
- Around line 89-103: The validation currently uses timeout.to_i and
retries.to_i which silently coerce invalid strings to 0; change to strict
numeric coercion by parsing timeout and retries with Integer/Float (e.g., parse
timeout into a float_ms = Float(timeout) rescue raise ArgumentError and parse
retries with Integer(retries) rescue raise ArgumentError), validate the parsed
values (timeout_ms > 0, retries >= 0), and then assign the parsed values to
client_config.connect_timeout / connection_request_timeout (use
parsed_timeout_ms / 1000.0) and client_config.max_retries (use parsed_retries)
so only valid numeric inputs reach ClientConfig.create and its attributes.
---
Nitpick comments:
In `@spec/context_spec.rb`:
- Around line 1324-1326: The test assigns an unused local original_experiments
after creating the context; remove the entire assignment (the variable
original_experiments) so the spec reads only the relevant setup (leave the
create_context call and any uses of context.experiments intact), ensuring no
other assertions rely on original_experiments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8626971-3f28-47c5-8c93-865a85d5c08e
📒 Files selected for processing (11)
lib/a_b_smartly.rblib/audience_matcher.rblib/client.rblib/context.rblib/context_event_logger_callback.rblib/default_audience_deserializer.rblib/json_expr/operators/match_operator.rbspec/concurrency_spec.rbspec/context_spec.rbspec/create_context_async_spec.rbspec/fix_plan_spec.rb
🚧 Files skipped from review as they are similar to previous changes (5)
- spec/create_context_async_spec.rb
- lib/audience_matcher.rb
- lib/client.rb
- lib/context_event_logger_callback.rb
- lib/default_audience_deserializer.rb
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/context.rb (1)
577-633:⚠️ Potential issue | 🔴 CriticalPrevent partially initialised context data from becoming visible to readers.
assign_datamutates shared lookup tables in place. With async creation/refresh, readers can observe mixed old/new state during rebuild. Build new maps locally, then swap all state atomically.Suggested fix
def assign_data(data) - `@data` = data - `@index` = {} - `@index_variables` = {} + new_index = {} + new_index_variables = {} + new_context_custom_fields = {} if data && !data.experiments.nil? && !data.experiments.empty? data.experiments.each do |experiment| `@experiment_custom_field_values` = {} @@ - variables.keys.each do |key| - `@index_variables`[key] ||= [] - `@index_variables`[key] << experiment_variables unless `@index_variables`[key].include?(experiment_variables) + variables.keys.each do |key| + new_index_variables[key] ||= [] + new_index_variables[key] << experiment_variables unless new_index_variables[key].include?(experiment_variables) end @@ - `@index`[experiment.name.to_sym] = experiment_variables - `@context_custom_fields`[experiment.name] = `@experiment_custom_field_values` + new_index[experiment.name.to_sym] = experiment_variables + new_context_custom_fields[experiment.name] = `@experiment_custom_field_values` end end end + + `@index` = new_index + `@index_variables` = new_index_variables + `@context_custom_fields` = new_context_custom_fields + `@data` = data end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/context.rb` around lines 577 - 633, assign_data currently mutates shared instance maps (`@index`, `@index_variables`, `@context_custom_fields`, `@experiment_custom_field_values`) in place which can expose partially-built state; change assign_data to build local hashes (e.g., local_index, local_index_variables, local_context_custom_fields) and local experiment-level structures while iterating experiments and only assign these locals to the instance variables (`@index` = local_index; `@index_variables` = local_index_variables; `@context_custom_fields` = local_context_custom_fields) once the entire new state is fully constructed to ensure an atomic swap and avoid readers seeing mixed old/new state; keep per-experiment ExperimentVariables creation and parsing logic but push results into the locals instead of the shared instance fields during build.
♻️ Duplicate comments (1)
lib/context.rb (1)
658-661:⚠️ Potential issue | 🟡 MinorInitialise
@indexas a Hash for consistent type semantics.
experimentuses hash-style lookups, butinit_commonsets@index = []. This creates an inconsistent type before first data assignment.Suggested fix
- `@index` = [] + `@index` = {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/context.rb` around lines 658 - 661, The `@index` instance variable is initialized as an Array in init_common which conflicts with later hash-style lookups (see experiment method); change the initialization in init_common (method name: init_common) from an array to a Hash (initialize `@index` as an empty hash) so `@index` has consistent hash semantics before any data is assigned.
🧹 Nitpick comments (1)
spec/fix_plan_spec.rb (1)
101-104: Strengthen the exposure assertion to catch false negatives.
<= 10can still pass when no exposure is queued. Add a lower bound so the test validates that queueing actually happened.Suggested fix
- expect(exposures.length).to be <= 10 + expect(exposures.length).to be_between(1, 10).inclusive🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/fix_plan_spec.rb` around lines 101 - 104, The test currently only asserts an upper bound on exposures (expect(exposures.length).to be <= 10) which can pass when nothing was queued; update the assertion to require a positive lower bound as well. In the spec/fix_plan_spec.rb example block where exposures is retrieved from context.instance_variable_get(:`@exposures`), replace the single upper-bound assertion with a range check such as expect(exposures.length).to be_between(1, 10).inclusive (or two assertions expect(...).to be >= 1 and expect(...).to be <= 10) so the test verifies that at least one exposure was queued and no more than 10.
🤖 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/context.rb`:
- Around line 360-399: The flush method races with producers
(track/queue_exposure) because it snapshots and clears `@exposures`, `@achievements`
and `@pending_count` without synchronization; introduce a shared mutex (e.g.
`@mutex` = Mutex.new) and wrap the critical sections in flush where
exposures/achievements/ pending_count are read/cleared and similarly wrap the
code paths in track and queue_exposure that mutate those instance variables so
producers and flush acquire the same lock to avoid lost events; ensure you only
hold the lock for the minimal snapshot/clear operations (release before calling
external handlers like `@event_handler.publish`) and update references to
methods/vars: flush, track, queue_exposure, `@exposures`, `@achievements`,
`@pending_count`, `@event_handler.publish`.
In `@spec/fix_plan_spec.rb`:
- Around line 155-160: The test "does not use Timeout.timeout" currently
short-circuits when Timeout is undefined; change it so it always inspects the
source of the MatchOperator implementation instead of relying on
defined?(Timeout). Update the example in the spec (the it block named "does not
use Timeout.timeout") to unconditionally read the contents of the MatchOperator
source (match_operator.rb) and assert that it does not include the string
"Timeout.timeout" (e.g., expect(source).not_to include("Timeout.timeout")),
removing the defined?(Timeout) branch so the file content is always checked.
---
Outside diff comments:
In `@lib/context.rb`:
- Around line 577-633: assign_data currently mutates shared instance maps
(`@index`, `@index_variables`, `@context_custom_fields`,
`@experiment_custom_field_values`) in place which can expose partially-built
state; change assign_data to build local hashes (e.g., local_index,
local_index_variables, local_context_custom_fields) and local experiment-level
structures while iterating experiments and only assign these locals to the
instance variables (`@index` = local_index; `@index_variables` =
local_index_variables; `@context_custom_fields` = local_context_custom_fields)
once the entire new state is fully constructed to ensure an atomic swap and
avoid readers seeing mixed old/new state; keep per-experiment
ExperimentVariables creation and parsing logic but push results into the locals
instead of the shared instance fields during build.
---
Duplicate comments:
In `@lib/context.rb`:
- Around line 658-661: The `@index` instance variable is initialized as an Array
in init_common which conflicts with later hash-style lookups (see experiment
method); change the initialization in init_common (method name: init_common)
from an array to a Hash (initialize `@index` as an empty hash) so `@index` has
consistent hash semantics before any data is assigned.
---
Nitpick comments:
In `@spec/fix_plan_spec.rb`:
- Around line 101-104: The test currently only asserts an upper bound on
exposures (expect(exposures.length).to be <= 10) which can pass when nothing was
queued; update the assertion to require a positive lower bound as well. In the
spec/fix_plan_spec.rb example block where exposures is retrieved from
context.instance_variable_get(:`@exposures`), replace the single upper-bound
assertion with a range check such as expect(exposures.length).to be_between(1,
10).inclusive (or two assertions expect(...).to be >= 1 and expect(...).to be <=
10) so the test verifies that at least one exposure was queued and no more than
10.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 88140906-475c-4d24-b315-20a1aeec2800
📒 Files selected for processing (2)
lib/context.rbspec/fix_plan_spec.rb
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/a_b_smartly.rb (1)
93-103:⚠️ Potential issue | 🟠 MajorUse
Integer()andFloat()for strict parameter coercion.The validation on lines 93-94 uses
.to_i, which allows non-numeric strings like"abc"to pass because"abc".to_ireturns0. The raw value is then forwarded toclient_config.max_retrieson line 103.This was previously flagged and remains unaddressed.
,🤖 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 93 - 103, The timeout and retries validation uses .to_i/.to_f which silently accepts non-numeric strings; replace those coercions with strict Integer(timeout) and Integer(retries) (rescuing ArgumentError to raise a clear ArgumentError if coercion fails) and use Float(timeout) for connect_timeout/connection_request_timeout; then assign the validated integer to client_config.max_retries and the validated float (divided by 1000.0) to client_config.connect_timeout and client_config.connection_request_timeout so invalid inputs like "abc" will trigger an ArgumentError instead of being treated as 0.
🧹 Nitpick comments (3)
lib/context_event_handler.rb (1)
3-6: Consider adding a runtime deprecation warning for consistency.Unlike
DefaultContextEventHandler, this alias doesn't emit a runtime deprecation warning. Users subclassingContextEventHandlerwon't be notified of the deprecation unless they read the source code.If runtime warnings are desired, you could use Ruby's
deprecate_constant:ContextEventHandler = ContextPublisher deprecate_constant :ContextEventHandlerThis is optional since the alias preserves full compatibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/context_event_handler.rb` around lines 3 - 6, The file creates an alias ContextEventHandler = ContextPublisher but doesn't emit a runtime deprecation warning; add a runtime deprecation for the legacy constant by invoking Ruby's deprecate_constant on ContextEventHandler after the alias so users get a runtime warning when referencing ContextEventHandler while preserving compatibility with ContextPublisher.lib/default_context_publisher.rb (1)
5-14: Consider usingattr_readerinstead ofattr_accessorforclient.The
clientis assigned once during initialisation. Usingattr_readerwould prevent unintended reassignment and better express the immutable intent.💡 Suggested change
class DefaultContextPublisher < ContextPublisher - attr_accessor :client + attr_reader :client def initialize(client) `@client` = client end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/default_context_publisher.rb` around lines 5 - 14, Replace the mutable accessor with a read-only accessor: change attr_accessor :client to attr_reader :client in the DefaultContextPublisher class so the client is only set in initialize and cannot be reassigned; verify no external code relies on a writer for DefaultContextPublisher#client and, if any does, refactor those call sites to avoid mutating the client or provide an explicit method to replace it with clear intent.lib/a_b_smartly.rb (1)
49-51: Minor style: Remove blank line afterprivateand fix indentation.RuboCop flags style issues with the access modifier layout.
💡 Suggested change
private - - def initialize_from_config(config) + def initialize_from_config(config)🤖 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 49 - 51, Remove the blank line after the access modifier `private` and adjust indentation so `def initialize_from_config(config)` is indented to the same level as `private` (no extra leading spaces), ensuring the `private` block and the `initialize_from_config` method follow RuboCop's access modifier layout style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/a_b_smartly.rb`:
- Around line 93-103: The timeout and retries validation uses .to_i/.to_f which
silently accepts non-numeric strings; replace those coercions with strict
Integer(timeout) and Integer(retries) (rescuing ArgumentError to raise a clear
ArgumentError if coercion fails) and use Float(timeout) for
connect_timeout/connection_request_timeout; then assign the validated integer to
client_config.max_retries and the validated float (divided by 1000.0) to
client_config.connect_timeout and client_config.connection_request_timeout so
invalid inputs like "abc" will trigger an ArgumentError instead of being treated
as 0.
---
Nitpick comments:
In `@lib/a_b_smartly.rb`:
- Around line 49-51: Remove the blank line after the access modifier `private`
and adjust indentation so `def initialize_from_config(config)` is indented to
the same level as `private` (no extra leading spaces), ensuring the `private`
block and the `initialize_from_config` method follow RuboCop's access modifier
layout style.
In `@lib/context_event_handler.rb`:
- Around line 3-6: The file creates an alias ContextEventHandler =
ContextPublisher but doesn't emit a runtime deprecation warning; add a runtime
deprecation for the legacy constant by invoking Ruby's deprecate_constant on
ContextEventHandler after the alias so users get a runtime warning when
referencing ContextEventHandler while preserving compatibility with
ContextPublisher.
In `@lib/default_context_publisher.rb`:
- Around line 5-14: Replace the mutable accessor with a read-only accessor:
change attr_accessor :client to attr_reader :client in the
DefaultContextPublisher class so the client is only set in initialize and cannot
be reassigned; verify no external code relies on a writer for
DefaultContextPublisher#client and, if any does, refactor those call sites to
avoid mutating the client or provide an explicit method to replace it with clear
intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aaee8212-6a3b-43ef-9521-82fa4b41c6d3
📒 Files selected for processing (5)
lib/a_b_smartly.rblib/context_event_handler.rblib/context_publisher.rblib/default_context_event_handler.rblib/default_context_publisher.rb
- Use Integer()/Float() for strict parameter coercion instead of .to_i/.to_f to prevent non-numeric strings from passing validation - Initialize @index as Hash to match its actual usage pattern - Replace return with next inside map block to prevent early method exit
The IN operator is a CONTAINS check with haystack-first operand order ([haystack, needle]), matching the collector and production behavior. A prior change on this branch had swapped the operands to needle-first, diverging from the collector. Revert the operator and its tests to the canonical haystack-first order.
…ndling) A null operand short-circuits binary operators to null — eq(null, null) is null, not true. This matches origin/main of every SDK and the collector. The branch had introduced an eq override (or removed the base null-skip) that made eq(null,null) true, diverging from the canonical behavior. Revert to the null-skip behavior and align the operator tests.
Pedro-Revez-Silva
left a comment
Pedro-Revez-Silva
left a comment
There was a problem hiding this comment.
Approved per request. Good functional coverage and multi-Ruby CI are present, but audit gaps remain: HTTP tests mostly assert retry/config plumbing and response factories, not collector wire shape; RuboCop/static config exists but is not run in CI; and the 137/138 cross-SDK claim is only in PR metadata, not backed by harness evidence in this PR.
Local server + public SDK exercises real GET /context and PUT /context, asserting the wire contract.
|
@Pedro-Revez-Silva Addressed the HTTP wire-shape gap. Real HTTP integration test: added `spec/local_server_integration_spec.rb` (commit `b2ccac2`), a stdlib `TCPServer` on an ephemeral port (WEBrick isn't bundled in Ruby 3.3). It drives the public SDK (`ABSmartly.new(endpoint, api_key:, ...) → create_context → wait_until_ready → track → publish`) so the real Faraday client makes an actual GET /context and PUT /context, and asserts the concrete collector wire shape (method/path/query and the publish headers + body), not just retry/config plumbing. CI already runs on `pull_request`. |
|
Review submitted The automated review has been submitted. See the review for the verdict and any findings. Head: |
jervasion-absmartly
left a comment
jervasion-absmartly
left a comment
There was a problem hiding this comment.
CHANGES REQUESTED - concurrent publish and refresh can expose or duplicate event/data state
Reviewed head: 78f49e4031064dd53e4cb493e740b65e82572f24
Merge-base: 2b09db3d1ab5e7d735d523d93ca99533e97f0634 (the supplied base 0fef1d1e542b44163c6be2113763381bdb715c6e is an ancestor; review covers the complete supplied base..head range)
Intent and impact: this PR adds asynchronous context creation/readiness signalling, context refresh changes, and queue synchronization intended to preserve events under concurrent operations. The changed surfaces are public Context APIs and the collector publish path.
Findings
-
[P2] Make a publish snapshot single-owner before sending it —
Context#publishis public and the newflushsnapshots the queues under@queue_mutex, but releases that mutex before@event_handler.publishand does not mark the snapshot in flight. Two concurrent callers can therefore both copy the same@exposures/@achievements, both send identical PUT payloads, and both later shift the same prefix. This is a reachable chain:Context#publish-> two application threads ->flushlines 374-394 -> shared event handler -> duplicate collector events. The existingspec/concurrency_spec.rbexercises concurrent publish calls but only asserts that they do not raise, so it does not catch the duplicate payload. Preserve the invariant that each queued event is included in at most one successful publish (and remains queued after a failed publish), using the smallest ownership/serialization mechanism that satisfies that invariant. -
[P2] Publish a fully built refresh state atomically —
assign_datanow resets and repopulates@index,@index_variables, and@context_custom_fieldsin place.refreshcalls it without the@ready_mutex, while public readers such astreatment,variable_keys, and custom-field access can run concurrently. During a refresh, a reader can observe the new@datawith only part of the new indexes, or the new index with stale/partially rebuilt companion maps, and compute a wrong treatment/variable/custom-field result. The reachable chain isContext#refresh->assign_datalines 590-646 -> concurrent public reader -> mixed lookup state. Build the replacement maps off to the side and publish the coherent set only after construction, or otherwise ensure readers cannot observe the intermediate state; the acceptance criterion is that every reader sees either the complete old snapshot or the complete new snapshot.
Validation
- Exact-head GitHub Actions CI: all six RSpec matrix jobs (Ruby 2.7, 3.0, 3.1, 3.2, 3.3, 3.4) completed successfully in run
39392977757. - Local syntax checks passed:
ruby -c lib/context.rb,ruby -c lib/a_b_smartly.rb,ruby -c lib/client.rb, andruby -c lib/json_expr/operators/match_operator.rb. git diff --checkpassed.- Full local Bundler/RSpec execution was not available because the environment could not write to the system gem cache and the provided
/tmp/opencodedirectory was root-owned; CI was used for the full suite. - No cloud queries, browser authentication, deployments, or PR source changes were used.
Passes (non-blocking): run: core runtime/API/concurrency/security; tests, CI, packaging, and end-to-end harnesses; cross-cutting simplicity; behaviour-bearing data and policy; comment hygiene and diff composition | not run: deployment, operations, and observability (no deployment manifests, release playbooks, service definitions, schema migrations, alerts, dashboards, runbooks, capacity constants, or new metrics/log/trace emission changed), shared-component blast radius and interaction complexity (no frontend source changed)
Comment hygiene (non-blocking): 26 comment / 2714 code lines, ratio ~1:104, 0 blocks flagged
threshold T=28; restatements counted: no
- none flagged
Diff composition (non-blocking): every file is required for the change; none flagged
Simplifications (non-blocking): 1 candidate
- nested duplicate
if event_count > 0guard inlib/context.rb, smaller alternative: remove the inner redundant guard while preserving the surrounding condition, LOC delta: -2
Run a simplification pass locally before pushing: these candidates cost less to address now than as a further review round.
Round 2: publish ownership fixed and validated; async parsing readiness fixed. The earlier broader refresh-atomicity requirement was pre-existing and over-scoped. No prior code blocker remains; separate merge conflict is documented in the updated review.
jervasion-absmartly
left a comment
jervasion-absmartly
left a comment
There was a problem hiding this comment.
COMMENT - prior code blockers cleared; merge conflicts prevent exact-head merge-readiness approval.
Round 2 assessment
Reviewed head a847fa78215227801e21bec469458bc4c155576e against base 0fef1d1e542b44163c6be2113763381bdb715c6e; merge-base / responsibility baseline 2b09db3d1ab5e7d735d523d93ca99533e97f0634. Correction to my previous metadata: the supplied base is not an ancestor of the head.
The follow-up serializes publish ownership and builds replacement context indexes locally. The cumulative PR changes Ruby compatibility, JSON expressions, public SDK convenience/async APIs, tests and documentation. No rollout flag is involved. Historical cross-SDK totals are author-reported, not independently rerun here.
Prior findings:
- Concurrent publishing: fixed. The publish mutex covers snapshot creation, the request, and successful removal. An overlapping-publish harness sent all 20 goals exactly once across five callers; failed-publish retention and retry also passed.
- Partial initialization: the new async path remains unready throughout parsing, with data published after the completed maps. A parser-blocking harness verified safe pre-ready reads and no premature exposure. Scope correction: concurrent refresh already rebuilt state in place at the merge-base. Requiring a globally atomic multi-map refresh/read transaction was not established as a PR-introduced obligation. Sequential instance-variable assignments are not an atomic transaction; I am not certifying that stronger property or retaining it as a blocker.
No verified code blocker survives. The supplied prior change-request review is being dismissed as addressed, with the scope correction above.
Remaining merge limitation
GitHub reports mergeable: false / CONFLICTING. Read-only git merge-tree --write-tree 0fef1d1e542b44163c6be2113763381bdb715c6e a847fa78215227801e21bec469458bc4c155576e independently reports a content conflict in lib/context.rb. Resolve it and validate the resulting head before treating the PR as ready to merge. This is not a CI deferral: exact-head CI has finished successfully.
Validation
- Installed locked dependencies with
BUNDLE_PATH=/workspace/review-gems BUNDLE_FROZEN=true bundle install --jobs 4, without changing the lockfile. BUNDLE_PATH=/workspace/review-gems BUNDLE_FROZEN=true bundle exec rspec: 487 examples, 0 failures, including real local-HTTP fetch/publish, async creation, failures and concurrency.- Independent runtime/tests pass: 54 focused examples and 487 full-suite examples passed; the new publish uniqueness test fails when the previous flush implementation is restored in memory.
- Primary-agent in-memory harness: async parsing/readiness, overlapping publish uniqueness, failed-publish retention and retry all passed.
- Exact-head CI run
39403028894: all six Ruby 2.7–3.4 RSpec jobs completed successfully. CodeRabbit status is success with review paused, not evidence of a new code review. git diff --checkpassed. RuboCop onlib/context.rb,spec/concurrency_spec.rb, andspec/fix_plan_spec.rbreported 13 style/unused-local offenses, not a clean lint result. None is introduced by this follow-up; these do not establish a functional blocker, and CI does not enforce RuboCop.
Passes (non-blocking)
Run: core runtime/API/security/data/error paths (SDK executable changes; independent remediation pass); tests/CI/packaging/end-to-end harnesses (new concurrency test and cumulative packaging/HTTP coverage); deployment/operations/observability (changed warning emissions in the cumulative diff); cross-cutting simplicity (mutex/local state and redundant-guard removal); comment hygiene and diff composition (cumulative diff and introduced-blob enumeration).
Not run: shared-component blast radius (no frontend source); behaviour-bearing data/policy (no runtime configuration, policy, schema or behavioural fixture change).
Comment hygiene (non-blocking)
29 comment / 2739 code lines, ratio ~1:94.
threshold T=28; restatements counted: no (10 × C < E).
Flagged restatement blocks: lib/context.rb:372–373, 407–408, 649; lib/context_publisher.rb:4; spec/local_server_integration_spec.rb:9–13, 94, 103, 111, 121. These describe adjacent implementation/test steps. Non-blocking census only; no promotion threshold is reached. Protocol/encoding constraints and Ruby magic comments are preserved.
Diff composition (non-blocking)
Every file is required for the change; none flagged. Introduced content writes were enumerated using the pinned policy helper, including intermediate history.
Simplifications (non-blocking)
None remaining from the prior review: the nested duplicate event-count guard was removed. No further concrete construct-removal candidate demonstrated.
No PR source changes, cloud queries, browser authentication or deployments were performed. Temporary review checkout and validation artifacts will be removed after publication.
jervasion-absmartly
left a comment
jervasion-absmartly
left a comment
There was a problem hiding this comment.
CHANGES REQUESTED - round 3 still has two verified concurrency defects
Reviewed head: 293849d71f08fd8a70dc9bd7e474ad498fcd39de
Merge-base: 0fef1d1e542b44163c6be2113763381bdb715c6e
Intent and impact
The PR adds asynchronous context creation/readiness APIs, refresh/index rebuilding, event-queue synchronization, public SDK conveniences, JSON-expression fixes, and broad compatibility/documentation/test coverage. The round-2 publish-ownership remediation is present at this head; the PR is otherwise cleanly mergeable according to GitHub.
Findings
Two P2 findings are posted inline. Both are reachable from public Context APIs and need to be fixed before merge:
-
Refresh still does not publish one coherent reader-visible state.
assign_dataconstructs replacement maps locally, but assigns@index,@index_variables,@context_custom_fields, and@dataas four separate instance-variable writes. Readers do not take a common snapshot/read lock. Duringrefresh, a reader can combine the new experiment index with the old variable index (or vice versa), returning a default or queuing an exposure for a state that never existed as a complete context snapshot. This is a remediation follow-up to the prior refresh-state finding; the stable acceptance criterion is that every reader observes either the complete old state or the complete new state. -
Exposure logging runs while
@queue_mutexis held. A configured event logger is user code; if its exposure callback calls publicpublishortrack, that callback recursively acquires the non-reentrant queue mutex and raisesThreadError: deadlock; recursive locking. The base invoked this callback without the queue mutex, so this is a remediation regression from the queue synchronization fix. The acceptance criterion is that event callbacks cannot deadlock or fail solely because queue bookkeeping is synchronized.
Validation
- Exact-head Actions: CI run
39455428270completed successfully; both the pull-request and push runs for this exact SHA are successful. The readable commit status is CodeRabbitsuccess(review paused), not treated as code validation. git diff --check: passed.- Ruby syntax:
ruby -c lib/context.rbandruby -c lib/a_b_smartly.rb: passed. - Independent adversarial pass reproduced the callback deadlock and reproduced mixed refresh state with a controlled refresh/read interleaving.
- Full local Bundler/RSpec execution was unavailable:
bundle exec rspecreports therspecexecutable is not installed, and the environment lacks the locked dependencies. - Introduced-content audit completed with the pinned helper; no credential, scratch artifact, or other unnecessary introduced content was found.
- No PR source changes, deployments, cloud queries, or browser authentication were used.
Passes (non-blocking)
Run: core runtime/API/security/data/error paths (executable SDK and concurrency changes); tests/CI/packaging/end-to-end harnesses (large test additions and exact-head CI); deployment/operations/observability (changed warning/logging behavior); cross-cutting simplicity (mutexes and snapshot publication); comment hygiene and diff composition (cumulative diff and introduced-content audit) | not run: shared-component blast radius (no frontend source changed), behaviour-bearing data and policy (no runtime configuration, policy, schema, or behaviour fixture changed)
Comment hygiene (non-blocking): 29 comment / 2739 code lines, ratio ~1:94, 9 blocks flagged
threshold T=28; restatements counted: no (10 × C < E)
lib/context.rb:372-373,407-408,649: restates the synchronization/snapshot codelib/context_publisher.rb:4: restates the aliasspec/local_server_integration_spec.rb:9-13,94,103,111,121: restate adjacent test setup/assertion steps
None warrants promotion; the threshold is not reached.
Diff composition (non-blocking): every file is required for the change; none flagged
Simplifications (non-blocking): none
No prior review was dismissed: the supplied dismissible review ID list did not include an active CHANGES_REQUESTED review ID, so no dismissal was attempted.
jervasion-absmartly
left a comment
jervasion-absmartly
left a comment
There was a problem hiding this comment.
CHANGES REQUESTED - round 4 leaves two verified reentrant/concurrency defects
Reviewed head: 69122e6842bd45315f9ebdd64b9e322a274bbf5d
Merge-base: 0fef1d1e542b44163c6be2113763381bdb715c6e
Intent and impact
The latest commit remediates the prior round's two blockers by introducing a State snapshot and moving exposure logger callbacks outside @queue_mutex. The cumulative PR changes public Context readiness, refresh, assignment, publishing, SDK configuration, JSON-expression behavior, and compatibility APIs. No rollout flag is involved; the changed Context APIs and collector path are public SDK surfaces.
Findings
Two P2 findings survive, both reachable from public Context APIs:
-
Remediation regression:
@datais still published separately from@state. The newStateswap is atomic for derived maps, butassign_dataassigns@stateand then@dataseparately.ready?,experiments,custom_field_keys, anddataread@datadirectly, while assignment and variable readers read@state. A refresh interleaving can therefore return new derived indexes with old data, or old derived indexes with new data; failure publication has the inverse ordering. The stable acceptance criterion from the prior round remains open: every reader must observe one complete old or complete new context state. -
Remediation regression:
PUBLISHcallbacks can deadlock on@publish_mutex.flushholds@publish_mutexwhile callinglog_event(PUBLISH, event). The logger callback is user code, and a callback calling publicpublishattempts to acquire the same non-reentrant mutex again and deadlocks. The latest test covers only anEXPOSUREcallback after@queue_mutexrelease, not reentrantPUBLISHcallbacks. The acceptance criterion is that event callbacks cannot deadlock or fail solely because queue bookkeeping/publish serialization is synchronized.
The other prior blocker, exposure callbacks under @queue_mutex, is satisfied by the latest code and test. I did not dismiss review PRR_kwDOHzET7s8AAAABTStoDA because the prompt did not supply it as an exact dismissible ID for an active review.
Validation
- Exact-head Actions runs
39475569363(pull request) and39475563233(push), each completed successfully with all six Ruby 2.7–3.4 jobs passing. - Commit status for the exact head is CodeRabbit
success/ review paused; it is not treated as code validation. git diff --checkpassed.ruby -c lib/context.rb,ruby -c lib/a_b_smartly.rb, andruby -c lib/json_expr/operators/match_operator.rbpassed.- Independent adversarial analysis verified the remaining
@state/@datasplit and the reentrantPUBLISHmutex path. - Full local RSpec could not be run because the checkout environment lacks the RSpec executable and locked dependencies; exact-head CI supplied the full-suite evidence.
- Introduced-content audit completed with the pinned helper; no credential, scratch artifact, build output, or unnecessary file was found.
- No PR source changes, deployments, cloud queries, or browser authentication were used.
Passes (non-blocking)
Run: core runtime/API/security/data/error paths (public SDK and concurrency changes); tests/CI/packaging/end-to-end harnesses (new reentrancy tests and exact-head CI); deployment/operations/observability (changed event/log callback behavior); cross-cutting simplicity (State snapshot and mutex lifecycle); comment hygiene and diff composition (cumulative diff and introduced-content audit) | not run: shared-component blast radius (no frontend source changed), behaviour-bearing data and policy (no runtime configuration, policy, schema, or behavior fixture changed)
Comment hygiene (non-blocking): 32 comment / 2779 code lines, ratio ~1:87, 11 blocks flagged
threshold T=28; restatements counted: no (10 × C < E)
lib/context.rb:13-14,230,378-379,413-414,656-656: restates adjacent synchronization/snapshot codelib/context_publisher.rb:4: restates the aliasspec/local_server_integration_spec.rb:9-13,94,103,111,121: restate adjacent test setup/assertion steps
None warrants promotion; the threshold is not reached.
Diff composition (non-blocking): every file is required for the change; none flagged
Simplifications (non-blocking): none
Convergence summary — review round 4
- Unresolved original blockers: 0
- Remediation regressions: 2
- Independent PR-change findings: 0
- Acceptance criteria still open: coherent
State/data publication; reentrant event callbacks must not deadlock - Human adjudication requested: no — both defects are verified and actionable
jervasion-absmartly
left a comment
jervasion-absmartly
left a comment
There was a problem hiding this comment.
CHANGES REQUESTED - round 5 still exposes reentrant lifecycle deadlocks and event loss
Reviewed head: 51703118b7ff748d9cd17f01f4e4ffe474ba115a
Merge-base: 0fef1d1e542b44163c6be2113763381bdb715c6e
Intent and impact
The latest commit moves all data-derived readers to State and adds an owned-mutex fast path intended to prevent nested PUBLISH callbacks from deadlocking. The cumulative PR changes public asynchronous readiness, refresh, assignment, publishing, event logging, SDK configuration, JSON-expression behavior, and compatibility APIs. The changed Context and event-logger APIs are public SDK surfaces; no rollout flag is involved.
Findings
Two P2 remediation regressions survive:
-
READY callbacks can deadlock async readiness.
set_dataholds@ready_mutexwhile invoking the configured event logger forREADY. A logger callback that calls the publicwait_until_readymethod re-enters the same non-reentrant mutex on the same thread and deadlocks, leaving async context creation stuck. The added callback coverage exercises exposure and PUBLISH events, but not READY callbacks. The acceptance criterion is that a user event callback cannot deadlock readiness or leave the context unable to become ready solely because readiness bookkeeping is synchronized. -
A nested
closefrom a PUBLISH callback can strand events in a closed context. The newreturn nil if @publish_mutex.owned?avoids recursive publish deadlock, butclosetreats that no-op flush as success and sets@closed = true. If the PUBLISH callback queues an event and callsclose, the outer publish removes only its original snapshot; the newly queued event remains pending, while subsequentpublishcalls reject the finalized context. This violates the documented close behavior that pending data is published. The acceptance criterion is that reentrant lifecycle calls cannot close a context while newly queued events remain permanently unsendable.
These are remediation regressions from the readiness/publish synchronization fixes. The prior @data/@state and direct PUBLISH deadlock findings are satisfied at this head. I did not dismiss any review: no exact dismissible CHANGES_REQUESTED review ID was supplied.
Validation
- Exact-head Actions runs
39491876585(pull request) and39491869063(push) completed successfully; all six Ruby 2.7–3.4 jobs passed. - Exact-head commit status is CodeRabbit
success/ review paused; it is not treated as code validation. git diff --checkpassed.ruby -c lib/context.rb,ruby -c lib/a_b_smartly.rb, andruby -c lib/json_expr/operators/match_operator.rbpassed.- Independent adversarial analysis verified the READY mutex re-entry path and the nested-close event-retention path from the current source.
- Full local RSpec was unavailable because the checkout lacks the RSpec executable and locked dependencies; exact-head CI supplied the full-suite evidence.
- Introduced-content audit completed with the pinned helper; no credential, scratch artifact, build output, or unnecessary file was found.
- No PR source changes, deployments, cloud queries, or browser authentication were used.
Passes (non-blocking)
Run: core runtime/API/security/data/error paths (public Context lifecycle and event callbacks); tests/CI/packaging/end-to-end harnesses (new callback tests and exact-head CI); deployment/operations/observability (changed event/log callback behavior); cross-cutting simplicity (State snapshot and mutex ownership); comment hygiene and diff composition (cumulative diff and introduced-content audit) | not run: shared-component blast radius (no frontend source changed), behaviour-bearing data and policy (no runtime configuration, policy, schema, or behavior fixture changed)
Comment hygiene (non-blocking): 36 comment / 2822 code lines, ratio ~1:78, 10 blocks flagged
threshold T=29; restatements counted: no (10 × C < E)
lib/context.rb:13-14,230,378-382,413-414,656-656: restates adjacent synchronization/snapshot codelib/context_publisher.rb:4: restates the aliasspec/local_server_integration_spec.rb:9-13,94,103,111,121: restate adjacent test setup/assertion steps
None warrants promotion; the threshold is not reached.
Diff composition (non-blocking): every file is required for the change; none flagged
Simplifications (non-blocking): none
Convergence summary — review round 5
- Unresolved original blockers: 0
- Remediation regressions: 2
- Independent PR-change findings: 0
- Acceptance criteria still open: reentrant READY callbacks must not deadlock; reentrant close must not strand queued events
- Human adjudication requested: yes — rounds 4 and 5 each replaced fixed blockers with new remediation regressions; please choose among fixing these lifecycle invariants, explicitly accepting the specific risks, or changing the callback/close contract.
…il publish completes
jervasion-absmartly
left a comment
jervasion-absmartly
left a comment
There was a problem hiding this comment.
APPROVED - Exact head is ready to merge; no verified blocker or material limitation remains.
Reviewed head: 042711a20ff7864cc992c1d7f95f0f96bc33a602
Merge-base: 0fef1d1e542b44163c6be2113763381bdb715c6e
The PR intends to bring the Ruby SDK to Ruby 3.3 compatibility and cross-SDK behavior, while adding async context creation and public API conveniences. The affected runtime surfaces are context readiness, immutable context-data publication, exposure/goal queueing and serialized publishing, callback reentrancy/close behavior, HTTP retry/publish handling, JSON-expression conversion, and the public SDK constructors and aliases. The PR reports 412/412 unit tests and 137/138 cross-SDK scenarios, with one documented async scenario skipped.
I re-read the cumulative merge-base-to-head changes, all available PR discussion and prior review threads, the current implementation, and exact-head CI. The final concurrency fixes preserve atomic state publication, invoke READY and EXPOSURE callbacks outside their respective mutexes, avoid nested publish deadlock, and defer close until an outer publish has completed and callback-queued events have been flushed. The adversarial pass found no verified surviving finding; a forced assignment-cache race was confirmed pre-existing at the base and is not attributed to this PR's changed behavior.
Validation:
- Exact-head GitHub Actions CI: the latest CI run for this head completed successfully for Ruby 2.7, 3.0, 3.1, 3.2, 3.3, and 3.4; each job's RSpec step passed.
ruby -c lib/context.rb,ruby -c lib/a_b_smartly.rb, andruby -c lib/client.rb: passed.git diff --check: passed.- Introduced-content audit over every PR commit: completed successfully; no credential, scratch artifact, build output, or unnecessary introduced content was found.
- Local Bundler/RSpec execution was attempted but could not install locked dependencies because the review environment's system gem cache is not writable. This is covered by the passing exact-head CI evidence above.
Passes (non-blocking): run: core runtime/API/security/data/error paths; tests, CI, packaging, and end-to-end harnesses; cross-cutting simplicity; comment hygiene and diff composition | not run: deployment, operations, and observability (no deployment manifests, release playbooks, service definitions, migrations, alerts, dashboards, runbooks, capacity constants, or metric/log/trace emission changed); shared-component blast radius and interaction complexity (no frontend source changed); behaviour-bearing data and policy (no runtime configuration, policy, generated contract/schema, or behavior-bearing fixture changed)
Comment hygiene (non-blocking): 38 comment / 2702 code lines, ratio ~1:71, 0 blocks flagged
- none flagged
threshold T=28; restatements counted: no (10 × C < E)
Diff composition (non-blocking): every file is required for the change; none flagged
Simplifications (non-blocking): none
Convergence summary — review round 6
- Unresolved original blockers: 0
- Remediation regressions: 0
- Independent PR-change findings: 0
- Acceptance criteria still open: none
- Human adjudication requested: no
3 participants
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
New Features
Documentation
Bug Fixes
Tests
Chores