Rust SDK: All tests passing (302/302 unit, 138/138 cross-SDK)#2
Rust SDK: All tests passing (302/302 unit, 138/138 cross-SDK)#2joalves wants to merge 21 commits into
Conversation
Replace MD5 (2→14), Murmur3 (9→36), VariantAssigner (4→43) with macro-generated canonical tests. Add 75 context tests and 19 SDK/client tests covering event logger, state management, treatment/peek/variable operations, publish/finalize/refresh lifecycle, cache invalidation, and custom fields.
- Fix context.rs: add error handling for variant config parsing, finalization guards on set_override/set_overrides, robust goal serialization, improved custom field number/json/boolean parsing - Fix jsonexpr/operators/mod.rs: handle edge cases in operator evaluation - Fix matcher.rs: improve audience matching robustness - Fix sdk.rs: strengthen SDK initialization and context management Results: 302/302 unit tests, 138/138 cross-SDK scenarios, exit code 0
|
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 pull request converts the SDK to an async-first, builder-based API by introducing ABsmartly and ABsmartlyBuilder for fluent configuration, replacing SDK::new with builder/new entry points, and redesigning context creation to fetch ContextData remotely with retry logic via backoff_ms. Extends Context with lifecycle methods (become_ready, become_failed, ready_error), state fields (failed_error, publish_delay, refresh_period), data fetcher integration for dynamic refresh, and publisher notification via ContextPublisher trait with DefaultContextPublisher. JSON expression operators are refactored: containment logic moves from in_op to contains_op with reversed (haystack-first) operand order; match_op gains RegexBuilder with explicit limits and error logging; comparison operators adopt is_some_and pattern; evaluator dispatch is updated to route both "in" and "contains" keys to contains_op. AudienceMatcher adds diagnostic logging for parse errors and filter validation. Test suites across assigner, murmur3, and md5 are refactored to use parameterised macros for expanded coverage. Cargo.toml adds tokio "time" feature and new dependencies (urlencoding, log). README is extensively rewritten with async examples and builder documentation. GitHub Actions workflows update Rust toolchain installation action. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…rate ContextOptions (publish_delay, refresh_period, event_logger) was silently unused. Now properly applied when creating contexts. Replace eprintln! calls with log::warn!/error! for proper library use.
…ssages - Add close() as alias for finalize() - Add is_closed() as alias for is_finalized() - Add is_closing() as alias for is_finalizing() - Standardize "ABsmartly Context is finalizing." to "ABsmartly Context is finalized." - Add unit tests for aliases and error messages
…fresh() Nine tests were calling the internal refresh_with(data) method directly, bypassing the data fetcher layer. Updated all refresh tests to use set_data_fetcher() with a mock closure and call the public refresh() API.
…ring all assignments
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/context.rs (1)
564-584:⚠️ Potential issue | 🔴 CriticalDo not clear the queue before publish has actually succeeded.
pending,exposures, andgoalsare reset before the publisher runs. WithDefaultContextPublisher, the PUT is spawned fire-and-forget and its result is ignored, so a network failure—or no Tokio runtime at all—silently loses those events.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.rs` around lines 564 - 584, The code clears the queue (self.pending, self.exposures, self.goals) before the publish actually completes, risking silent loss on network/runtime failure; change the flow in Context::publish so you build params with build_publish_params() and serialize/log first, then call publisher.publish(¶ms) and only clear self.pending, self.exposures, and self.goals after publisher.publish returns success (or the spawned future resolves successfully) — for synchronous publishers check the Result and clear on Ok, for async/spawned publishers await the future or change the DefaultContextPublisher to return a Result/Future so Context::publish can detect failure and avoid clearing on error; keep log_event and serde_json::to_value handling but move the clearing to after a successful publish.src/sdk.rs (1)
209-244:⚠️ Potential issue | 🟠 Major
retries(0)skips the initial fetch entirely.The loop is
while retries > 0, so zero configured retries means zero attempts rather than “one attempt, no retries”. That turns a valid configuration into an immediate error without ever touching the network.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sdk.rs` around lines 209 - 244, The loop currently uses let mut retries = self.config.retries.unwrap_or(5) and while retries > 0 which makes a configured 0 mean “no attempts”; change the semantics so configured retries means “number of retries after the first attempt.” Capture the original configured value (e.g., let configured_retries = self.config.retries.unwrap_or(5);), then set let mut attempts_left = configured_retries + 1 and loop while attempts_left > 0 (or use a for/loop with break), decrement attempts_left on each failed attempt, and keep using configured_retries when computing backoff_ms so backoff_ms(configured_retries, attempts_left - 1) or equivalent; update references to retries, backoff_ms, and last_error accordingly to ensure zero configured retries still performs one network request.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Line 24: The Tokio dependency in Cargo.toml is missing the "time" feature used
by src/sdk.rs (calls to tokio::time::sleep at ~lines 229 and 240); update the
tokio dependency entry to include "time" in the features array so the time
module is compiled (i.e., change the tokio = { version = "1.0", features =
["rt-multi-thread"] } entry to include "time"). Ensure the features list
contains both "rt-multi-thread" and "time" so tokio::time::sleep in src/sdk.rs
works for downstream builds.
In `@README.md`:
- Around line 125-133: The README example uses context.refresh(fresh_data) but
Context::refresh() is the no-arg method; replace the call with
context.refresh_with(fresh_data) and mention the correct by-value API
(refresh_with) when showing ContextData deserialization so the example compiles;
update the snippet to call refresh_with on the Context instance instead of
refresh.
In `@src/context.rs`:
- Around line 74-99: The constructors and state transition methods (new,
new_with_options, become_ready and any other code paths that set self.state =
ContextState::Error) update state but never emit lifecycle events; call the
existing log_event method after changing state to notify listeners: after
setting self.state = ContextState::Ready in new, new_with_options and
become_ready invoke self.log_event("ready", ...) with appropriate payload, and
likewise ensure any place that assigns self.state = ContextState::Error calls
self.log_event("error", ...) with the error details so event_logger receives the
lifecycle events.
In `@src/jsonexpr/operators/mod.rs`:
- Around line 92-101: Add a regression test that exercises the invalid-regex
branch in match_op: call the match_op (or the public operator function that
invokes RegexBuilder) with a text value and a deliberately invalid pattern like
"(", and assert the returned Value is Value::Null; put the test alongside the
other operator tests (create a new #[test] that constructs the operand Values,
calls match_op, and asserts equality with Value::Null) so the behavior of the
RegexBuilder Err path is locked in.
In `@src/models.rs`:
- Around line 161-166: ContextOptions is non-Clone because event_logger is
Box<dyn Fn...>, which forces callers to rebuild options; change the event_logger
field to Option<Arc<dyn Fn(&crate::context::Context, &str,
Option<serde_json::Value>) + Send + Sync + 'static>> and derive/implement Clone
for ContextOptions so instances can be cheaply shared; update uses/imports to
include std::sync::Arc and adjust any construction sites to wrap callbacks with
Arc::new(...) rather than Box::new(...).
In `@src/sdk.rs`:
- Around line 298-311: The refresh fetcher closure installed via
context.set_data_fetcher contains a blocking rt.block_on on
tokio::runtime::Handle::try_current() which will panic when invoked from async
contexts; change the implementation to use tokio::task::block_in_place to create
a blocking section and call Handle::current().block_on(...) inside it so the
fetcher can be invoked from async tasks safely. Specifically, update the closure
passed to context.set_data_fetcher (the refresh fetcher used by
Context::refresh) to wrap the inner block_on call in tokio::task::block_in_place
and switch from try_current() to Handle::current(), preserving the existing
error mapping (e.g., map_err messages) for both the HTTP request
(client.get...send) and JSON parse (resp.json::<ContextData>()) so behavior and
errors remain the same.
- Around line 88-90: The builder's agent(...) value (config.agent) is never
applied to outgoing HTTP requests; update each request construction path that
currently sets "X-API-Key" and "Content-Type" (the request-building code used in
the methods that set those headers) to also set the User-Agent header when
config.agent.is_some(): read config.agent, and if present call the request
builder's header method to add "User-Agent" (or header::USER_AGENT) with the
stored string; keep the existing timeout logic and ensure agent() remains as the
setter for config.agent so the header uses the same stored value.
---
Outside diff comments:
In `@src/context.rs`:
- Around line 564-584: The code clears the queue (self.pending, self.exposures,
self.goals) before the publish actually completes, risking silent loss on
network/runtime failure; change the flow in Context::publish so you build params
with build_publish_params() and serialize/log first, then call
publisher.publish(¶ms) and only clear self.pending, self.exposures, and
self.goals after publisher.publish returns success (or the spawned future
resolves successfully) — for synchronous publishers check the Result and clear
on Ok, for async/spawned publishers await the future or change the
DefaultContextPublisher to return a Result/Future so Context::publish can detect
failure and avoid clearing on error; keep log_event and serde_json::to_value
handling but move the clearing to after a successful publish.
In `@src/sdk.rs`:
- Around line 209-244: The loop currently uses let mut retries =
self.config.retries.unwrap_or(5) and while retries > 0 which makes a configured
0 mean “no attempts”; change the semantics so configured retries means “number
of retries after the first attempt.” Capture the original configured value
(e.g., let configured_retries = self.config.retries.unwrap_or(5);), then set let
mut attempts_left = configured_retries + 1 and loop while attempts_left > 0 (or
use a for/loop with break), decrement attempts_left on each failed attempt, and
keep using configured_retries when computing backoff_ms so
backoff_ms(configured_retries, attempts_left - 1) or equivalent; update
references to retries, backoff_ms, and last_error accordingly to ensure zero
configured retries still performs one network request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41e6c287-f9e5-4455-b29e-12f912a970dd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlREADME.mdsrc/assigner.rssrc/context.rssrc/context_publisher.rssrc/jsonexpr/operators/mod.rssrc/lib.rssrc/matcher.rssrc/md5.rssrc/models.rssrc/murmur3.rssrc/sdk.rs
| ### Refreshing the Context with Fresh Experiment Data | ||
|
|
||
| You can add additional units to a context by calling the `set_unit()` method. This method may be used, for example, when a user logs in to your application, and you want to use the new unit type in the context. | ||
|
|
||
| **Note:** You cannot override an already set unit type as that would be a change of identity. In this case, you must create a new context instead. | ||
| For long-running contexts, the context can be refreshed manually with updated experiment data: | ||
|
|
||
| ```rust | ||
| context.set_unit("db_user_id", "1000013").unwrap(); | ||
| let fresh_data: ContextData = serde_json::from_str(&api_response)?; | ||
|
|
||
| context.refresh(fresh_data); | ||
| ``` |
There was a problem hiding this comment.
Use refresh_with in the example.
Context::refresh() takes no arguments and pulls via the configured fetcher; the by-value API is refresh_with(fresh_data). As written, this snippet does not compile.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 125 - 133, The README example uses
context.refresh(fresh_data) but Context::refresh() is the no-arg method; replace
the call with context.refresh_with(fresh_data) and mention the correct by-value
API (refresh_with) when showing ContextData deserialization so the example
compiles; update the snippet to call refresh_with on the Context instance
instead of refresh.
| pub fn new(data: ContextData) -> Self { | ||
| let mut ctx = Self::base(); | ||
| ctx.init(data); | ||
| ctx.state = ContextState::Ready; | ||
| ctx | ||
| } | ||
|
|
||
| pub fn new_with_options(data: ContextData, options: ContextOptions) -> Self { | ||
| let mut ctx = Self::base(); | ||
| ctx.event_logger = options.event_logger; | ||
| ctx.publish_delay = options.publish_delay; | ||
| ctx.refresh_period = options.refresh_period; | ||
| ctx.init(data); | ||
| ctx.state = ContextState::Ready; | ||
| ctx | ||
| } | ||
|
|
||
| pub fn new_loading() -> Self { | ||
| Self::base() | ||
| } | ||
|
|
||
| pub fn become_ready(&mut self, data: ContextData) { | ||
| if self.state == ContextState::Loading { | ||
| self.init(data); | ||
| self.state = ContextState::Ready; | ||
| } |
There was a problem hiding this comment.
The event logger never sees the documented ready/error lifecycle events.
These state transitions update the context, but none of them call log_event("ready", ...) or log_event("error", ...). Consumers wiring event_logger will therefore never observe the two lifecycle events now advertised by the public API.
Also applies to: 108-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/context.rs` around lines 74 - 99, The constructors and state transition
methods (new, new_with_options, become_ready and any other code paths that set
self.state = ContextState::Error) update state but never emit lifecycle events;
call the existing log_event method after changing state to notify listeners:
after setting self.state = ContextState::Ready in new, new_with_options and
become_ready invoke self.log_event("ready", ...) with appropriate payload, and
likewise ensure any place that assigns self.state = ContextState::Error calls
self.log_event("error", ...) with the error details so event_logger receives the
lifecycle events.
- Add tokio time feature to Cargo.toml for downstream builds - Fix block_in_place pattern to avoid panic in async contexts - Apply User-Agent header from agent config to all HTTP requests - Change EventLogger type to Arc for Clone-able ContextOptions - Add test for invalid regex pattern returning Null in match_op
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/sdk.rs (2)
287-290:⚠️ Potential issue | 🟠 Major
agent()still misses the context publisher path.
src/context_publisher.rs@ lines 12-17 showsDefaultContextPublisher::new(...)only receives endpoint and api_key, so the publisher installed here cannot forward the configuredUser-Agent. Direct SDK requests are fixed now, butContext::publish()andfinalize()remain inconsistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sdk.rs` around lines 287 - 290, The context publisher installed in agent() via context.set_publisher(Box::new(DefaultContextPublisher::new(&self.config.endpoint, &self.config.api_key))) doesn't receive the configured User-Agent, so Context::publish() and Context::finalize() can't forward it; update the call to DefaultContextPublisher::new to accept and pass the User-Agent (or full headers) from self.config (e.g., self.config.user_agent) and adjust DefaultContextPublisher::new and its constructor signature in src/context_publisher.rs to store and use that User-Agent when sending context, ensuring context.set_publisher, Context::publish(), and Context::finalize() all use the same publisher behavior.
301-318:⚠️ Potential issue | 🔴 Critical
refresh()can still panic outside a multi-thread Tokio runtime.Lines 302-303 assume both
block_in_placesupport and a current runtime handle. AContextcreated viacreate_context_with()can be refreshed from plain synchronous code, and a current-thread runtime will panic here as well. Please either make refresh async end-to-end, or fall back to a private runtime when no suitable Tokio runtime is active.Tokio 1.x docs: does `tokio::task::block_in_place` require the multi-thread runtime, and does `tokio::runtime::Handle::current()` panic outside a runtime?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sdk.rs` around lines 301 - 318, The data fetcher closure used by context.set_data_fetcher (and the refresh() path) assumes a multi-thread Tokio runtime and a current Handle, which can panic for callers created via create_context_with() running in sync or current-thread runtimes; update the closure to first attempt a non-panicking runtime handle (e.g., tokio::runtime::Handle::try_current()) and, if successful, use block_in_place + handle.block_on as before, but if no suitable handle exists fall back to creating a short-lived private Tokio runtime (tokio::runtime::Runtime::new or Builder) and use runtime.block_on to perform the async request; alternatively make refresh() async end-to-end and remove blocking ops—implement the fallback in the closure around the client.get/send/json sequence referenced in the diff so refresh() no longer panics when no multi-thread runtime is active.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/context.rs`:
- Around line 318-331: The set_override and set_overrides functions currently
allow mutations after finalization; update both to mirror other mutating Context
APIs by first checking self.is_finalized() and self.is_finalizing() and
returning an Err(String) (with a clear message) if either is true, before
performing any inserts or calling set_override; ensure set_overrides uses the
guarded set_override path so both functions refuse changes once finalization has
started or completed.
In `@src/sdk.rs`:
- Around line 281-285: The loop that calls Context::set_unit for each
(unit_type, uid) currently only logs errors and continues, so create_context can
return a partially-initialized Context; change the loop to propagate failures by
returning the error on the Result path instead of just warn!-ing: when
context.set_unit(&unit_type, &uid) yields Err(e) in the loop, return Err(e) (or
map/convert e into the function's error type) so create_context (or the
surrounding function) fails fast; ensure you update any function signature/err
conversions as needed and keep references to Context::set_unit, units, and
create_context consistent.
---
Duplicate comments:
In `@src/sdk.rs`:
- Around line 287-290: The context publisher installed in agent() via
context.set_publisher(Box::new(DefaultContextPublisher::new(&self.config.endpoint,
&self.config.api_key))) doesn't receive the configured User-Agent, so
Context::publish() and Context::finalize() can't forward it; update the call to
DefaultContextPublisher::new to accept and pass the User-Agent (or full headers)
from self.config (e.g., self.config.user_agent) and adjust
DefaultContextPublisher::new and its constructor signature in
src/context_publisher.rs to store and use that User-Agent when sending context,
ensuring context.set_publisher, Context::publish(), and Context::finalize() all
use the same publisher behavior.
- Around line 301-318: The data fetcher closure used by context.set_data_fetcher
(and the refresh() path) assumes a multi-thread Tokio runtime and a current
Handle, which can panic for callers created via create_context_with() running in
sync or current-thread runtimes; update the closure to first attempt a
non-panicking runtime handle (e.g., tokio::runtime::Handle::try_current()) and,
if successful, use block_in_place + handle.block_on as before, but if no
suitable handle exists fall back to creating a short-lived private Tokio runtime
(tokio::runtime::Runtime::new or Builder) and use runtime.block_on to perform
the async request; alternatively make refresh() async end-to-end and remove
blocking ops—implement the fallback in the closure around the
client.get/send/json sequence referenced in the diff so refresh() no longer
panics when no multi-thread runtime is active.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e048dc86-5f41-41b3-a233-71822db3ddff
📒 Files selected for processing (5)
Cargo.tomlsrc/context.rssrc/jsonexpr/operators/mod.rssrc/models.rssrc/sdk.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/jsonexpr/operators/mod.rs
| pub fn set_override(&mut self, experiment_name: &str, variant: i32) -> Result<(), String> { | ||
| self.overrides.insert(experiment_name.to_string(), variant); | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn set_overrides<I, K>(&mut self, overrides: I) | ||
| pub fn set_overrides<I, K>(&mut self, overrides: I) -> Result<(), String> | ||
| where | ||
| I: IntoIterator<Item = (K, i32)>, | ||
| K: Into<String>, | ||
| { | ||
| for (experiment_name, variant) in overrides { | ||
| self.overrides.insert(experiment_name.into(), variant); | ||
| self.set_override(&experiment_name.into(), variant)?; | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
set_override(s) bypass the finalisation guard.
These methods are now fallible, but Lines 318-331 never check is_finalized() or is_finalizing(). That lets callers change treatment resolution after finalize(), unlike the other mutating APIs on Context.
Suggested fix
pub fn set_override(&mut self, experiment_name: &str, variant: i32) -> Result<(), String> {
+ if self.is_finalized() {
+ return Err("ABsmartly Context is finalized.".to_string());
+ }
+ if self.is_finalizing() {
+ return Err("ABsmartly Context is finalized.".to_string());
+ }
self.overrides.insert(experiment_name.to_string(), variant);
Ok(())
}📝 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.
| pub fn set_override(&mut self, experiment_name: &str, variant: i32) -> Result<(), String> { | |
| self.overrides.insert(experiment_name.to_string(), variant); | |
| Ok(()) | |
| } | |
| pub fn set_overrides<I, K>(&mut self, overrides: I) | |
| pub fn set_overrides<I, K>(&mut self, overrides: I) -> Result<(), String> | |
| where | |
| I: IntoIterator<Item = (K, i32)>, | |
| K: Into<String>, | |
| { | |
| for (experiment_name, variant) in overrides { | |
| self.overrides.insert(experiment_name.into(), variant); | |
| self.set_override(&experiment_name.into(), variant)?; | |
| } | |
| Ok(()) | |
| pub fn set_override(&mut self, experiment_name: &str, variant: i32) -> Result<(), String> { | |
| if self.is_finalized() { | |
| return Err("ABsmartly Context is finalized.".to_string()); | |
| } | |
| if self.is_finalizing() { | |
| return Err("ABsmartly Context is finalized.".to_string()); | |
| } | |
| self.overrides.insert(experiment_name.to_string(), variant); | |
| Ok(()) | |
| } | |
| pub fn set_overrides<I, K>(&mut self, overrides: I) -> Result<(), String> | |
| where | |
| I: IntoIterator<Item = (K, i32)>, | |
| K: Into<String>, | |
| { | |
| for (experiment_name, variant) in overrides { | |
| self.set_override(&experiment_name.into(), variant)?; | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/context.rs` around lines 318 - 331, The set_override and set_overrides
functions currently allow mutations after finalization; update both to mirror
other mutating Context APIs by first checking self.is_finalized() and
self.is_finalizing() and returning an Err(String) (with a clear message) if
either is true, before performing any inserts or calling set_override; ensure
set_overrides uses the guarded set_override path so both functions refuse
changes once finalization has started or completed.
| for (unit_type, uid) in units { | ||
| let _ = context.set_unit(&unit_type, &uid); | ||
| if let Err(e) = context.set_unit(&unit_type, &uid) { | ||
| warn!("Failed to set unit '{}': {}", unit_type, e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fail fast when a supplied unit is invalid.
Context::set_unit() already rejects blank/conflicting values, but Line 282 only logs the failure and still returns a ready context. That means create_context() can succeed with a partially initialised context and silently change assignment behaviour. Please propagate the error on the Result-returning path instead of returning Ok(...).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sdk.rs` around lines 281 - 285, The loop that calls Context::set_unit for
each (unit_type, uid) currently only logs errors and continues, so
create_context can return a partially-initialized Context; change the loop to
propagate failures by returning the error on the Result path instead of just
warn!-ing: when context.set_unit(&unit_type, &uid) yields Err(e) in the loop,
return Err(e) (or map/convert e into the function's error type) so
create_context (or the surrounding function) fails fast; ensure you update any
function signature/err conversions as needed and keep references to
Context::set_unit, units, and create_context consistent.
Align the IN operator with the collector and the other ABsmartly SDKs by using haystack-first operand order ([haystack, needle]) — IN is a CONTAINS check. Register a 'contains' operator key as an alias for the same implementation, keeping 'in' for backwards compatibility with existing audiences. Tests updated to the haystack-first convention.
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 (1)
src/jsonexpr/operators/mod.rs (1)
103-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid logging the evaluated regex pattern.
pattern_strcan be produced from a variable operand, so an invalid pattern may leak user/customer data into logs. Keep the compiler error, but omit or truncate the pattern value.Proposed fix
- Err(e) => { - error!("Invalid regex pattern '{}': {}", pattern_str, e); + Err(e) => { + error!("Invalid regex pattern: {}", e); Value::Null }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsonexpr/operators/mod.rs` around lines 103 - 106, The error logging statement in the regex error handler is logging the actual regex pattern value (pattern_str) which can contain sensitive user/customer data when derived from variable operands. Modify the error! macro call to remove pattern_str from the format string and log arguments. Keep the compiler error information (the variable e) in the log message, but omit or truncate the pattern_str reference entirely to prevent potential data leakage into logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/jsonexpr/operators/mod.rs`:
- Around line 103-106: The error logging statement in the regex error handler is
logging the actual regex pattern value (pattern_str) which can contain sensitive
user/customer data when derived from variable operands. Modify the error! macro
call to remove pattern_str from the format string and log arguments. Keep the
compiler error information (the variable e) in the log message, but omit or
truncate the pattern_str reference entirely to prevent potential data leakage
into logs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58ad1127-8d94-4a69-a6a1-99f2b1ebbeb2
📒 Files selected for processing (3)
src/jsonexpr/evaluator.rssrc/jsonexpr/operators/mod.rssrc/matcher.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/matcher.rs
- Replace the nonexistent dtolnay/rust-action@stable with dtolnay/rust-toolchain@master (the action reference CI failed to resolve). - Resolve clippy warnings (map_or simplifications, manual bit-rotation, derivable Default, iterating map keys, single-iteration loop) and extract a complex closure type into an EventLogger type alias. - Apply rustfmt. All 318 tests pass; clippy (-D warnings), fmt --check, and cargo doc are clean.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 26: Replace the moving target reference `@master` with an immutable
pinned version across all five dtolnay/rust-toolchain action uses to prevent
supply-chain security vulnerabilities and ensure build stability. In
.github/workflows/ci.yml at lines 26, 54, and 72, change each `uses:
dtolnay/rust-toolchain@master` to use a specific version tag (e.g., `@1.0.0`) or
commit SHA. In .github/workflows/publish.yml at lines 19 and 34, apply the same
pinning change to the dtolnay/rust-toolchain action references. Use the same
pinned version across all five locations for consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9f9a559-9396-4cc0-b970-d13bd67b2af9
📒 Files selected for processing (14)
.github/workflows/ci.yml.github/workflows/publish.ymlsrc/assigner.rssrc/context.rssrc/jsonexpr/evaluator.rssrc/jsonexpr/mod.rssrc/jsonexpr/operators/mod.rssrc/lib.rssrc/matcher.rssrc/md5.rssrc/models.rssrc/murmur3.rssrc/sdk.rssrc/utils.rs
✅ Files skipped from review due to trivial changes (2)
- src/utils.rs
- src/jsonexpr/mod.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/lib.rs
- src/md5.rs
- src/matcher.rs
- src/assigner.rs
- src/jsonexpr/operators/mod.rs
- src/context.rs
|
|
||
| - name: Install Rust | ||
| uses: dtolnay/rust-action@stable | ||
| uses: dtolnay/rust-toolchain@master |
There was a problem hiding this comment.
Pin dtolnay/rust-toolchain to specific version or commit SHA across all workflow jobs. All five action references use @master, which is a moving target that can introduce breaking changes or security vulnerabilities without notice. Pin to an immutable reference for stability and supply-chain security.
.github/workflows/ci.yml#L26-L26: pin the action reference in the test job to a specific version tag (e.g.,@1.0.0) or commit SHA..github/workflows/ci.yml#L54-L54: pin the action reference in the lint job to a specific version tag or commit SHA..github/workflows/ci.yml#L72-L72: pin the action reference in the docs job to a specific version tag or commit SHA..github/workflows/publish.yml#L19-L19: pin the action reference in the test job to a specific version tag or commit SHA..github/workflows/publish.yml#L34-L34: pin the action reference in the publish job to a specific version tag or commit SHA.
🧰 Tools
🪛 zizmor (1.25.2)
[error] 26-26: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
📍 Affects 2 files
.github/workflows/ci.yml#L26-L26(this comment).github/workflows/ci.yml#L54-L54.github/workflows/ci.yml#L72-L72.github/workflows/publish.yml#L19-L19.github/workflows/publish.yml#L34-L34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 26, Replace the moving target reference
`@master` with an immutable pinned version across all five
dtolnay/rust-toolchain action uses to prevent supply-chain security
vulnerabilities and ensure build stability. In .github/workflows/ci.yml at lines
26, 54, and 72, change each `uses: dtolnay/rust-toolchain@master` to use a
specific version tag (e.g., `@1.0.0`) or commit SHA. In
.github/workflows/publish.yml at lines 19 and 34, apply the same pinning change
to the dtolnay/rust-toolchain action references. Use the same pinned version
across all five locations for consistency.
Source: MCP tools
The matrix tested Rust 1.70.0, but the dependency tree and the v4 Cargo.lock require a newer toolchain (1.70 fails with "failed to parse lock file"). There is no declared MSRV and 1.70 never actually built. Test on stable across ubuntu/macos/windows; a real MSRV can be pinned later via rust-version if needed.
…ndling) A null operand short-circuits eq to null — it does not treat null == null as a match. This aligns with the other ABsmartly SDKs and the collector. Update the operator tests accordingly.
Pedro-Revez-Silva
left a comment
Pedro-Revez-Silva
left a comment
There was a problem hiding this comment.
Approved per request. Missing real HTTP coverage for the public async SDK paths. create_context/publish use reqwest, but tests are synchronous unit tests around builders/context state, with no mock server or tokio HTTP assertions. Please add deterministic tests for GET /context refresh/create and PUT /context publish through the public SDK.
Local server + public async SDK exercises real GET /context and PUT /context, asserting the wire contract. CI-runnable complement to the live-backend e2e.
publish() sent only X-API-Key, Content-Type, and User-Agent, omitting X-Application, X-Environment, X-Application-Version:0, and X-Agent that the collector and the other SDKs use. Add the missing headers and assert the full set in the integration test.
|
@Pedro-Revez-Silva Addressed — and your review caught a real bug. Real HTTP integration test: added `tests/http_integration.rs` (commit `f06ada8`), a tokio local server on 127.0.0.1, driving the public async SDK (`ABsmartly::builder()...build() → create_context().await → treatment + track → publish().await`) so the real reqwest client makes an actual GET /context and PUT /context, asserting the wire contract. CI (`ci.yml`) already runs on `push` + `pull_request`. Bug it surfaced (fixed in the same commit): the publish PUT only sent `X-API-Key` / `Content-Type` / `User-Agent`, omitting `X-Application`, `X-Environment`, `X-Application-Version:0`, and `X-Agent` that the collector and the other SDKs send. Added the missing headers; the test asserts the full set. |
2 participants
Summary
Changes
set_override/set_overrides, robust goal serialization, improved custom field number/json/boolean parsingTest Results
alpine:3.19todebian:bookworm-slim(glibc compatibility) in the cross-sdk-tests rust-wrapperTest plan
cargo test-- 302/302 passingSummary by CodeRabbit
ABsmartlySDK setup withABsmartlyBuilder, async context creation, and async publishing.containssupport (haystack-first) and enhanced audience/error logging.