ci: add strict clippy rules#3
Conversation
|
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 PR updates CI workflows and Rust toolchain/caching, adds rust-version and expanded linting in Cargo.toml, and bumps clippy msrv. It introduces new public modules (context, md5, murmur3, utils), many public data models (ContextData, ExperimentData, Variant, Assignment, Exposure, Attribute, Goal, Unit, PublishParams, ContextState, ContextParams, ContextOptions), a substantially expanded Context API (events, units, attributes, overrides, treatments, tracking, publish/finalize), exposes and extends the JSON expression Evaluator and operators, adds SDKConfig/SDK/SDKError with async context creation, and new utilities (base64, hash, variant selection). Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (4)
src/utils.rs (1)
19-28:⚠️ Potential issue | 🟡 MinorEdge case: empty split slice returns 0 instead of indicating an error.
When
splitis empty,choose_variantreturns0(fromsaturating_sub(1)on0). Depending on usage, this might silently produce an invalid variant index. If an empty split is considered a programming error, returning anOption<usize>or panicking with documentation would make the contract clearer.If this behaviour is intentional for graceful degradation, consider adding a brief doc comment noting the empty-slice semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.rs` around lines 19 - 28, The function choose_variant currently returns 0 for an empty split (due to split.len().saturating_sub(1)), which can silently produce an invalid index; update choose_variant to explicitly handle the empty-slice case—either change its signature to return Option<usize> and return None when split.is_empty(), or keep usize but explicitly panic (with a clear message) when split.is_empty(); implement an early check at the top of choose_variant (using the split parameter) and update all call sites to handle the new return type or accept the documented panic behavior, and add a short doc comment describing the chosen semantics.src/sdk.rs (1)
143-156:⚠️ Potential issue | 🟠 MajorCanonicalise units once, before both code paths use them.
build_context()trims and validates units throughContext::set_unit()and then ignores any error, whilecreate_context()serialises the raw input as-is.create_context_with()can silently drop bad units, andcreate_context()can return a localContextthat no longer matches the request the server evaluated.Also applies to: 227-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sdk.rs` around lines 143 - 156, The units are being canonicalised/validated inconsistently: build_context() calls Context::set_unit() but ignores errors and create_context() serialises the raw input directly; instead canonicalise and validate the units once up-front (using Context::set_unit() or the same trimming/validation logic) and collect the resulting canonical (trimmed/validated) unit list, propagate any validation errors instead of silencing them, and then use that canonical unit list to construct ContextRequest and UnitRequest in create_context(), create_context_with(), and build_context() so the server request matches the local Context state.src/context.rs (2)
349-359:⚠️ Potential issue | 🟠 MajorDo not queue exposures from a finalised context, or for an unknown experiment.
treatment()and the exposure path invariable_value()still flipexposed, append toself.exposures, and incrementpendingafterfinalize().treatment()also does it for names missing fromself.index, becauseassign()caches a defaultAssignmentfor misses, which turnstreatment("missing")into a bogus publishable exposure.Also applies to: 399-405
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.rs` around lines 349 - 359, treatment() and the exposure path in variable_value() currently mark assignments exposed, queue exposures, and increment pending even after finalize() or for experiments not present in self.index (assign() creates a default cached Assignment), producing invalid exposures; modify treatment(), variable_value(), and the shared exposure path so they first check that the context is not finalized (e.g., a finalized flag or finalize() state) and that the experiment_name exists in self.index before mutating self.assignments, self.exposures, or self.pending; specifically, in treatment() use assign() only to read assignment but guard the exposed flip / queue_exposure call by verifying self.index.contains_key(experiment_name) and that !self.finalized, and similarly ensure variable_value() follows the same guard so no exposures are queued or pending incremented for finalized contexts or unknown experiments.
730-738:⚠️ Potential issue | 🟠 MajorHash units when building the publish payload.
self.hashesis only populated on the assignment path. A goal-only flow such asset_unit()→track()→publish()therefore emitshashed: truewithuid: None, so the published goal is no longer attributable to the caller’s unit.Possible fix
- let units: Vec<Unit> = self - .units - .keys() - .map(|unit_type| Unit { - unit_type: unit_type.clone(), - uid: self.hashes.get(unit_type).cloned(), - }) - .collect(); + let units: Vec<Unit> = self + .units + .iter() + .map(|(unit_type, uid)| Unit { + unit_type: unit_type.clone(), + uid: self + .hashes + .get(unit_type) + .cloned() + .or_else(|| Some(hash_unit(uid))), + }) + .collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.rs` around lines 730 - 738, When building the publish payload in build_publish_params, units are created from self.units.keys() but self.hashes may be empty for goal-only flows (set_unit → track → publish), causing uid: None; update build_publish_params to ensure every Unit has a uid by checking self.hashes.get(unit_type) and, if missing, compute/insert the unit hash into self.hashes (using the existing hash-generation helper or entry API) and use that value for uid so Unit { unit_type, uid: Some(hash) } is emitted; reference build_publish_params, Unit, self.hashes, and the related flows set_unit/track/publish when making the fix.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
25-28: Consider using a stable reference for the toolchain action.Using
dtolnay/rust-toolchain@mastermay introduce unexpected breakages if the action's master branch changes. The lint and docs jobs correctly use@stable.♻️ Suggested change
- name: Install Rust - uses: dtolnay/rust-toolchain@master + uses: dtolnay/rust-toolchain@stable with: toolchain: ${{ matrix.rust }}Note: The
@stablereference still respects thetoolchaininput parameter, so this change would not affect the MSRV testing with1.70.0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 25 - 28, Change the GitHub Actions step that installs Rust which currently uses dtolnay/rust-toolchain@master (the "Install Rust" step) to reference a stable tag instead (e.g., dtolnay/rust-toolchain@stable) so the workflow uses a pinned, stable action; keep the existing toolchain input (matrix.rust) unchanged to preserve MSRV testing behavior.src/jsonexpr/evaluator.rs (1)
98-105: Avoid cloning the whole variable map for every lookup.
extract_var()materialises a temporaryValue::Objectfrom the entireself.varsmap before it even starts walking the path. In the audience-matching hot path that makes every{"var": ...}read O(n) in the number of attributes, even when the lookup is just a single top-level key.Possible refactor
- let frags: Vec<&str> = path.split('/').collect(); - let mut target: &Value = &Value::Object( - self.vars - .iter() - .map(|(k, v)| (k.clone(), v.clone())) - .collect(), - ); - - for frag in frags { + let mut frags = path.split('/'); + let Some(first) = frags.next() else { + return Value::Null; + }; + let mut target = match self.vars.get(first) { + Some(value) => value, + None => return Value::Null, + }; + + for frag in frags {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jsonexpr/evaluator.rs` around lines 98 - 105, The current extract_var builds a temporary Value::Object from self.vars for every lookup; instead, start by using the first path fragment to directly index into self.vars (e.g. let mut target: &Value = match self.vars.get(first_frag) { Some(v) => v, _ => &Value::Null }) and then walk remaining fragments without cloning the whole map; update extract_var to avoid creating Value::Object from self.vars, use self.vars.get(...) for the top-level lookup and then descend into nested Value objects/arrays as before, cloning only the final Value to return.
🤖 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 295-309: set_override, set_overrides, and refresh are mutating the
context after finalize() because they don't call check_not_finalized(); make
these methods refuse mutation post-finalize by invoking
self.check_not_finalized() at the start of each (i.e., add a call to
self.check_not_finalized()?; in set_override(&mut self,...), set_overrides(&mut
self,...), and the refresh(&mut self, ...) method referenced around lines
501-509) and return/propagate the error or panic consistent with
check_not_finalized()’s contract so mutations are blocked once finalize() has
been called.
In `@src/sdk.rs`:
- Around line 160-164: The loop treats `retries` as total attempts so calling
`with_retries(0)` skips the initial request; change the logic to treat `retries`
as the number of retry attempts after the first try (i.e., always perform one
initial attempt). In the function using `max_retries`, `retries`, and
`last_error` (and the `with_retries` caller), implement either:
attempt-first-then-retry loop (do an initial request, on failure check if
`retries == 0` then fail, otherwise decrement `retries` and backoff/retry) or
set `attempts = max_retries + 1` and loop while `attempts > 0` decrementing
after each attempt; apply the same fix to the similar block around the symbols
on lines ~197-200 to ensure `with_retries(0)` results in one attempt with no
backoff.
---
Outside diff comments:
In `@src/context.rs`:
- Around line 349-359: treatment() and the exposure path in variable_value()
currently mark assignments exposed, queue exposures, and increment pending even
after finalize() or for experiments not present in self.index (assign() creates
a default cached Assignment), producing invalid exposures; modify treatment(),
variable_value(), and the shared exposure path so they first check that the
context is not finalized (e.g., a finalized flag or finalize() state) and that
the experiment_name exists in self.index before mutating self.assignments,
self.exposures, or self.pending; specifically, in treatment() use assign() only
to read assignment but guard the exposed flip / queue_exposure call by verifying
self.index.contains_key(experiment_name) and that !self.finalized, and similarly
ensure variable_value() follows the same guard so no exposures are queued or
pending incremented for finalized contexts or unknown experiments.
- Around line 730-738: When building the publish payload in
build_publish_params, units are created from self.units.keys() but self.hashes
may be empty for goal-only flows (set_unit → track → publish), causing uid:
None; update build_publish_params to ensure every Unit has a uid by checking
self.hashes.get(unit_type) and, if missing, compute/insert the unit hash into
self.hashes (using the existing hash-generation helper or entry API) and use
that value for uid so Unit { unit_type, uid: Some(hash) } is emitted; reference
build_publish_params, Unit, self.hashes, and the related flows
set_unit/track/publish when making the fix.
In `@src/sdk.rs`:
- Around line 143-156: The units are being canonicalised/validated
inconsistently: build_context() calls Context::set_unit() but ignores errors and
create_context() serialises the raw input directly; instead canonicalise and
validate the units once up-front (using Context::set_unit() or the same
trimming/validation logic) and collect the resulting canonical
(trimmed/validated) unit list, propagate any validation errors instead of
silencing them, and then use that canonical unit list to construct
ContextRequest and UnitRequest in create_context(), create_context_with(), and
build_context() so the server request matches the local Context state.
In `@src/utils.rs`:
- Around line 19-28: The function choose_variant currently returns 0 for an
empty split (due to split.len().saturating_sub(1)), which can silently produce
an invalid index; update choose_variant to explicitly handle the empty-slice
case—either change its signature to return Option<usize> and return None when
split.is_empty(), or keep usize but explicitly panic (with a clear message) when
split.is_empty(); implement an early check at the top of choose_variant (using
the split parameter) and update all call sites to handle the new return type or
accept the documented panic behavior, and add a short doc comment describing the
chosen semantics.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 25-28: Change the GitHub Actions step that installs Rust which
currently uses dtolnay/rust-toolchain@master (the "Install Rust" step) to
reference a stable tag instead (e.g., dtolnay/rust-toolchain@stable) so the
workflow uses a pinned, stable action; keep the existing toolchain input
(matrix.rust) unchanged to preserve MSRV testing behavior.
In `@src/jsonexpr/evaluator.rs`:
- Around line 98-105: The current extract_var builds a temporary Value::Object
from self.vars for every lookup; instead, start by using the first path fragment
to directly index into self.vars (e.g. let mut target: &Value = match
self.vars.get(first_frag) { Some(v) => v, _ => &Value::Null }) and then walk
remaining fragments without cloning the whole map; update extract_var to avoid
creating Value::Object from self.vars, use self.vars.get(...) for the top-level
lookup and then descend into nested Value objects/arrays as before, cloning only
the final Value to return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f989940-16fb-444f-9750-4eb551b1af6f
📒 Files selected for processing (15)
.github/workflows/ci.ymlCargo.tomlclippy.tomlsrc/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
| /// Sets an override for the given experiment, forcing a specific variant. | ||
| pub fn set_override(&mut self, experiment_name: &str, variant: i32) { | ||
| self.overrides.insert(experiment_name.to_string(), variant); | ||
| let _ = self.overrides.insert(experiment_name.to_owned(), variant); | ||
| } | ||
|
|
||
| /// Sets multiple experiment overrides at once. | ||
| pub fn set_overrides<I, K>(&mut self, overrides: I) | ||
| where | ||
| I: IntoIterator<Item = (K, i32)>, | ||
| K: Into<String>, | ||
| { | ||
| for (experiment_name, variant) in overrides { | ||
| self.overrides.insert(experiment_name.into(), variant); | ||
| let _ = self.overrides.insert(experiment_name.into(), variant); | ||
| } | ||
| } |
There was a problem hiding this comment.
Make finalize() terminal for overrides and refreshes.
set_override(), set_overrides(), and refresh() still mutate the context without going through check_not_finalized(). After finalize(), callers can keep changing overrides or replace the experiment set entirely, so the lifecycle contract is not actually enforced.
Also applies to: 501-509
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/context.rs` around lines 295 - 309, set_override, set_overrides, and
refresh are mutating the context after finalize() because they don't call
check_not_finalized(); make these methods refuse mutation post-finalize by
invoking self.check_not_finalized() at the start of each (i.e., add a call to
self.check_not_finalized()?; in set_override(&mut self,...), set_overrides(&mut
self,...), and the refresh(&mut self, ...) method referenced around lines
501-509) and return/propagate the error or panic consistent with
check_not_finalized()’s contract so mutations are blocked once finalize() has
been called.
| let max_retries = self.config.retries.unwrap_or(5); | ||
| let mut retries = max_retries; | ||
| let mut last_error = None; | ||
|
|
||
| while retries > 0 { |
There was a problem hiding this comment.
Treat retries as retries, not total attempts.
With the current loop, with_retries(0) never sends the initial request and immediately falls back to "Failed to create context after retries". Zero retries should still mean one attempt with no backoff.
Possible fix
- let max_retries = self.config.retries.unwrap_or(5);
- let mut retries = max_retries;
+ let max_retries = self.config.retries.unwrap_or(5);
let mut last_error = None;
- while retries > 0 {
+ for attempt in 0..=max_retries {
let response = self
.client
.put(&url)
@@
Ok(resp) => {
if resp.status().is_success() {
let data: ContextData = resp.json().await?;
return Ok(Self::build_context(units_vec, data, options));
} else if resp.status().is_server_error() {
- retries -= 1;
if let Err(e) = resp.error_for_status() {
last_error = Some(SDKError::HttpError(e));
}
} else if let Err(e) = resp.error_for_status() {
return Err(SDKError::HttpError(e));
}
}
Err(e) => {
- retries -= 1;
last_error = Some(SDKError::HttpError(e));
}
}
- if retries > 0 {
- let backoff = 50 * 2_u64.pow(max_retries - retries);
+ if attempt < max_retries {
+ let backoff = 50 * 2_u64.pow(attempt + 1);
tokio::time::sleep(std::time::Duration::from_millis(backoff)).await;
}
}Also applies to: 197-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sdk.rs` around lines 160 - 164, The loop treats `retries` as total
attempts so calling `with_retries(0)` skips the initial request; change the
logic to treat `retries` as the number of retry attempts after the first try
(i.e., always perform one initial attempt). In the function using `max_retries`,
`retries`, and `last_error` (and the `with_retries` caller), implement either:
attempt-first-then-retry loop (do an initial request, on failure check if
`retries == 0` then fail, otherwise decrement `retries` and backoff/retry) or
set `attempts = max_retries + 1` and loop while `attempts > 0` decrementing
after each attempt; apply the same fix to the similar block around the symbols
on lines ~197-200 to ensure `with_retries(0)` results in one attempt with no
backoff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/models.rs (1)
241-246: GuardContextOptionsagainst negative timing values.Lines 243 and 245 use signed milliseconds. Negative values can propagate invalid scheduling state. Consider validating on construction (or moving to non-negative types) so bad config fails fast.
♻️ Proposed non-breaking guard
#[derive(Debug, Clone, Default)] pub struct ContextOptions { /// Delay in milliseconds before auto-publishing events. pub publish_delay: i64, /// Interval in milliseconds for automatic context refresh. pub refresh_period: i64, } + +impl ContextOptions { + pub fn validate(&self) -> Result<(), &'static str> { + if self.publish_delay < 0 { + return Err("publish_delay must be >= 0"); + } + if self.refresh_period < 0 { + return Err("refresh_period must be >= 0"); + } + Ok(()) + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models.rs` around lines 241 - 246, ContextOptions currently uses signed i64 for publish_delay and refresh_period which allows negative timings; add a validation step on construction (e.g., implement ContextOptions::new or a TryFrom/validate method) that checks publish_delay and refresh_period are >= 0 and returns an error (or panics) if not, or alternatively change the fields to non-negative types (e.g., u64) and update all callers to use the new constructor/convert paths; reference the ContextOptions struct and ensure callers that create ContextOptions use the validated constructor or handle the error.
🤖 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/models.rs`:
- Around line 187-193: The Unit struct is emitting "uid": null when a hash is
missing; update the Unit definition so the uid field is skipped during
serialization when None by adding the serde attribute (e.g.,
#[serde(skip_serializing_if = "Option::is_none")]) to the uid field in the Unit
struct, and ensure build_publish_params (the code that populates uid via
self.hashes.get(unit_type).cloned()) can continue to return None without
producing null in published payloads; alternatively, if you prefer strictness,
enforce hash computation before serialisation in build_publish_params rather
than changing the struct.
---
Nitpick comments:
In `@src/models.rs`:
- Around line 241-246: ContextOptions currently uses signed i64 for
publish_delay and refresh_period which allows negative timings; add a validation
step on construction (e.g., implement ContextOptions::new or a TryFrom/validate
method) that checks publish_delay and refresh_period are >= 0 and returns an
error (or panics) if not, or alternatively change the fields to non-negative
types (e.g., u64) and update all callers to use the new constructor/convert
paths; reference the ContextOptions struct and ensure callers that create
ContextOptions use the validated constructor or handle the error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec6f486d-df6d-451c-8396-a3420c561e54
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlsrc/models.rssrc/murmur3.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- Cargo.toml
- src/murmur3.rs
| pub struct Unit { | ||
| /// The unit type (e.g., `session_id`). | ||
| #[serde(rename = "type")] | ||
| pub unit_type: String, | ||
| /// The hashed unit identifier. | ||
| pub uid: Option<String>, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Locate Unit definition and UID type"
rg -nP --type=rust -C2 '\bstruct\s+Unit\b|uid\s*:\s*Option<String>|uid\s*:\s*String'
echo
echo "2) Find Unit literals with uid: None"
rg -nPU --type=rust -C2 'Unit\s*\{(?s).*?uid\s*:\s*None'
echo
echo "3) Find PublishParams construction and serialisation call-sites"
rg -nP --type=rust -C3 '\bPublishParams\b|serde_json::to_(string|vec|value)\s*\('Repository: absmartly/rust-sdk
Length of output: 734
🏁 Script executed:
# Find PublishParams definition to see which Unit it uses
rg -nP --type=rust 'struct\s+PublishParams' -A 15Repository: absmartly/rust-sdk
Length of output: 985
🏁 Script executed:
# Check which Unit is imported/used in different modules
rg -nP --type=rust 'use.*Unit|pub.*Unit' | head -20Repository: absmartly/rust-sdk
Length of output: 141
🏁 Script executed:
# Check if src/models.rs::Unit is actually exported or used
rg -nP --type=rust 'mod\s+models|from models|models::Unit'Repository: absmartly/rust-sdk
Length of output: 91
🏁 Script executed:
# View the context of both Unit definitions and their modules
head -80 src/sdk.rs | tail -20Repository: absmartly/rust-sdk
Length of output: 436
🏁 Script executed:
# Also check what's exported from models.rs
rg -nP --type=rust '^pub struct|^pub enum' src/models.rs | head -20Repository: absmartly/rust-sdk
Length of output: 423
🏁 Script executed:
# Find where PublishParams is serialized or used in network calls
rg -nP --type=rust 'PublishParams|serde_json::to_' -B2 -A5 | head -100Repository: absmartly/rust-sdk
Length of output: 3522
🏁 Script executed:
# Search for Unit construction to see if uid: None is ever used
rg -nP --type=rust 'Unit\s*\{' -A5Repository: absmartly/rust-sdk
Length of output: 650
🏁 Script executed:
# Check if there are any validation or assertion functions
rg -nP --type=rust 'fn.*validat|impl.*Unit|pub fn.*publish' -A3Repository: absmartly/rust-sdk
Length of output: 438
🏁 Script executed:
# Check what self.hashes contains and how it's populated
rg -nP --type=rust 'self\.hashes|hashes:\s*' -B3 -A3Repository: absmartly/rust-sdk
Length of output: 1804
🏁 Script executed:
# Check if there's any validation before publish
rg -nP --type=rust 'fn publish|validate|check' src/context.rs -A10 | head -50Repository: absmartly/rust-sdk
Length of output: 1597
Unit.uid optional field can emit null in publish payloads.
In build_publish_params() (context.rs:734–737), uid is populated via self.hashes.get(unit_type).cloned(), which returns None if a hash has not yet been computed for that unit type. When serialised, this produces "uid": null in the published payload. The pattern used elsewhere in PublishParams (e.g., for exposures, goals, attributes) shows awareness of #[serde(skip_serializing_if = "Option::is_none")]—apply the same here to prevent null emission.
🔧 Recommended fix (skip null on wire)
pub struct Unit {
/// The unit type (e.g., `session_id`).
#[serde(rename = "type")]
pub unit_type: String,
/// The hashed unit identifier.
+ #[serde(skip_serializing_if = "Option::is_none")]
pub uid: Option<String>,
}Alternatively, enforce that all units have computed hashes before serialisation.
📝 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 struct Unit { | |
| /// The unit type (e.g., `session_id`). | |
| #[serde(rename = "type")] | |
| pub unit_type: String, | |
| /// The hashed unit identifier. | |
| pub uid: Option<String>, | |
| } | |
| pub struct Unit { | |
| /// The unit type (e.g., `session_id`). | |
| #[serde(rename = "type")] | |
| pub unit_type: String, | |
| /// The hashed unit identifier. | |
| #[serde(skip_serializing_if = "Option::is_none")] | |
| pub uid: Option<String>, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/models.rs` around lines 187 - 193, The Unit struct is emitting "uid":
null when a hash is missing; update the Unit definition so the uid field is
skipped during serialization when None by adding the serde attribute (e.g.,
#[serde(skip_serializing_if = "Option::is_none")]) to the uid field in the Unit
struct, and ensure build_publish_params (the code that populates uid via
self.hashes.get(unit_type).cloned()) can continue to return None without
producing null in published payloads; alternatively, if you prefer strictness,
enforce hash computation before serialisation in build_publish_params rather
than changing the struct.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
26-28: Pin to the latest tagged release instead of@master.Using
@masterfordtolnay/rust-toolchaincan lead to non-reproducible builds if the action changes unexpectedly. Pin to a tagged release for better stability and consistency across builds.Suggested fix
- name: Install Rust - uses: dtolnay/rust-toolchain@master + uses: dtolnay/rust-toolchain@v1 with: toolchain: ${{ matrix.rust }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 26 - 28, Replace the action reference using dtolnay/rust-toolchain@master with a pinned tagged release (e.g., dtolnay/rust-toolchain@vX.Y.Z) to ensure reproducible CI; update the uses line that currently reads "uses: dtolnay/rust-toolchain@master" to point at the latest stable tag, keep "with: toolchain: ${{ matrix.rust }}" as-is, and update the tag when you intentionally want to upgrade the toolchain action.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 26-28: Replace the action reference using
dtolnay/rust-toolchain@master with a pinned tagged release (e.g.,
dtolnay/rust-toolchain@vX.Y.Z) to ensure reproducible CI; update the uses line
that currently reads "uses: dtolnay/rust-toolchain@master" to point at the
latest stable tag, keep "with: toolchain: ${{ matrix.rust }}" as-is, and update
the tag when you intentionally want to upgrade the toolchain action.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f0eaec2-a39c-452b-8527-d013f2180fc6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.github/workflows/ci.ymlCargo.tomlclippy.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- clippy.toml
calthejuggler
force-pushed
the
feat/strict-library-level-clippy-rules
branch
from
2698fd3 to
0d5c36c
Compare
calthejuggler
force-pushed
the
feat/strict-library-level-clippy-rules
branch
from
beeae77 to
76a0139
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
26-28: Pin the toolchain action to a specific tag or commit SHA for reproducible builds.Using
@masteris unstable and may cause unexpected CI failures when upstream changes. Consider pinning to a specific version tag (e.g.,@1.84.0or a commit SHA).🔧 Suggested fix
- name: Install Rust - uses: dtolnay/rust-toolchain@master + uses: dtolnay/rust-toolchain@stable with: toolchain: ${{ matrix.rust }}Alternatively, pin to a specific commit SHA for maximum reproducibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 26 - 28, Replace the unstable reference dtolnay/rust-toolchain@master in the GitHub Actions step with a pinned version (either a specific release tag like dtolnay/rust-toolchain@v1.84.0 or a commit SHA) to ensure reproducible CI; update the uses value that currently reads dtolnay/rust-toolchain@master so it points to the chosen tag or SHA (keeping the existing with: toolchain: ${{ matrix.rust }} line intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 26-28: Replace the unstable reference
dtolnay/rust-toolchain@master in the GitHub Actions step with a pinned version
(either a specific release tag like dtolnay/rust-toolchain@v1.84.0 or a commit
SHA) to ensure reproducible CI; update the uses value that currently reads
dtolnay/rust-toolchain@master so it points to the chosen tag or SHA (keeping the
existing with: toolchain: ${{ matrix.rust }} line intact).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cdff061-78dd-4732-a8ce-b3c19b5da811
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/ci.yml.gitignoreCargo.tomlclippy.toml
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- clippy.toml
calthejuggler
force-pushed
the
feat/strict-library-level-clippy-rules
branch
from
f4de142 to
bc457e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
20-20: Consider pinning lint/docs toolchains to MSRV for deterministic gating.With
cargo clippy ... -D warnings, floatingstablecan fail unrelated PRs when new warnings land. A common pattern is to gate on1.85.0and run a separate non-blocking latest-stable job.Also applies to: 44-44, 63-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml at line 20, The CI matrix currently uses a floating "stable" Rust toolchain under the rust matrix key which can make clippy/docs jobs non-deterministic; change the matrix entries that read rust: [stable, "1.85.0"] (and the other occurrences) to pin the linting/docs jobs to the MSRV only (e.g., rust: ["1.85.0"]) and, if you still want to exercise latest stable, add a separate non-blocking job that uses rust: [stable] so that cargo clippy -D warnings runs deterministically against MSRV while still reporting (but not failing) on the latest stable toolchain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 26: Replace all GitHub Action moving refs in .github/workflows/ci.yml
(e.g., the uses: dtolnay/rust-toolchain@master line and the other uses: entries
flagged at lines 30, 44, 48, 63, 65) with immutable commit SHAs; locate each
uses: "<owner>/<repo>@<ref>" occurrence that uses branches or tags like `@master`,
`@stable`, or `@v2` and pin them to the corresponding commit SHA from the action's
repository (update the ref to @<full-commit-sha>), ensuring every uses: entry in
the workflow is changed to a specific commit hash and not a moving ref.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 20: The CI matrix currently uses a floating "stable" Rust toolchain under
the rust matrix key which can make clippy/docs jobs non-deterministic; change
the matrix entries that read rust: [stable, "1.85.0"] (and the other
occurrences) to pin the linting/docs jobs to the MSRV only (e.g., rust:
["1.85.0"]) and, if you still want to exercise latest stable, add a separate
non-blocking job that uses rust: [stable] so that cargo clippy -D warnings runs
deterministically against MSRV while still reporting (but not failing) on the
latest stable toolchain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c8127b0-ee26-40df-b49c-5929a0c173db
📒 Files selected for processing (3)
.github/workflows/ci.ymlCargo.tomlclippy.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- clippy.toml
|
|
||
| - name: Install Rust | ||
| uses: dtolnay/rust-action@stable | ||
| uses: dtolnay/rust-toolchain@master |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify action refs that are not pinned to 40-char SHAs.
rg -nP '^\s*-\s*uses:\s*[^@]+@(?!(?:[0-9a-fA-F]{40})$).+$' .github/workflows/*.ymlRepository: absmartly/rust-sdk
Length of output: 576
🏁 Script executed:
sed -n '20,35p' .github/workflows/ci.ymlRepository: absmartly/rust-sdk
Length of output: 420
🏁 Script executed:
cat -n .github/workflows/ci.ymlRepository: absmartly/rust-sdk
Length of output: 2017
🏁 Script executed:
# Check if there are any commit SHAs already pinned in the file
rg -nP '@[0-9a-fA-F]{40}' .github/workflows/ci.ymlRepository: absmartly/rust-sdk
Length of output: 44
Pin GitHub Actions to immutable commit SHAs.
Using moving refs (@master, @stable, @v2) allows upstream changes to alter CI behaviour without review and weakens supply-chain guarantees.
Proposed hardening diff
- - name: Install Rust
- uses: dtolnay/rust-toolchain@master
+ - name: Install Rust
+ uses: dtolnay/rust-toolchain@<full_commit_sha>
- - uses: Swatinem/rust-cache@v2
+ - uses: Swatinem/rust-cache@<full_commit_sha>
- - name: Install Rust
- uses: dtolnay/rust-toolchain@stable
+ - name: Install Rust
+ uses: dtolnay/rust-toolchain@<full_commit_sha>
- - uses: Swatinem/rust-cache@v2
+ - uses: Swatinem/rust-cache@<full_commit_sha>
- - name: Install Rust
- uses: dtolnay/rust-toolchain@stable
+ - name: Install Rust
+ uses: dtolnay/rust-toolchain@<full_commit_sha>
- - uses: Swatinem/rust-cache@v2
+ - uses: Swatinem/rust-cache@<full_commit_sha>Also applies to: 30-30, 44-44, 48-48, 63-63, 65-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml at line 26, Replace all GitHub Action moving refs
in .github/workflows/ci.yml (e.g., the uses: dtolnay/rust-toolchain@master line
and the other uses: entries flagged at lines 30, 44, 48, 63, 65) with immutable
commit SHAs; locate each uses: "<owner>/<repo>@<ref>" occurrence that uses
branches or tags like `@master`, `@stable`, or `@v2` and pin them to the corresponding
commit SHA from the action's repository (update the ref to @<full-commit-sha>),
ensuring every uses: entry in the workflow is changed to a specific commit hash
and not a moving ref.
1 participant
This PR adds strict rules to
clippyto ensure the Rust SDK is performant, idiomatic, and well documented on crates.ioSummary by CodeRabbit
New Features
Build & CI