[init] initial implementation#1
Open
capcom6 wants to merge 5 commits into
Open
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIntroduces the ChangesSMSGate Rust client library
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Client
participant HttpTransport
participant SMSGateAPI
Caller->>Client: invoke API method (e.g., send, get_settings)
Client->>HttpTransport: build authenticated request
HttpTransport->>SMSGateAPI: send HTTP request with Bearer or Basic auth
SMSGateAPI-->>HttpTransport: JSON response + headers
HttpTransport->>HttpTransport: deserialize JSON or map HTTP error
HttpTransport-->>Client: typed result or Error variant
Client-->>Caller: return (data, total_count) or Error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (12)
Cargo.toml (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
tokiooptional dependency is never activated by any feature.The
tokiodependency at line 26 is markedoptional = truewithfeatures = ["macros"], but no feature in the[features]section enables it. It is only used as a dev-dependency (line 38) withfeatures = ["full"]. The optional non-dev declaration is dead configuration and should be removed.♻️ Proposed fix
-tokio = { version = "1", features = ["macros"], optional = true }🤖 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 `@Cargo.toml` at line 26, Remove the unused optional tokio dependency declaration from Cargo.toml, leaving the dev-dependency declaration intact since tokio is only used for development and no feature activates the optional entry.src/client.rs (1)
91-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated path+query construction logic.
The pattern of checking if a query string is empty and building a path with or without
?is repeated insend,list_messages, andlist_inbox_messages. Consider extracting a helper function to reduce duplication.♻️ Proposed helper extraction
+fn build_path(base: &str, query: &str) -> String { + if query.is_empty() { + base.to_string() + } else { + format!("{}?{}", base, query) + } +}Then use it in each method:
-let path = if query.is_empty() { - "/messages".to_string() -} else { - format!("/messages?{}", query) -}; +let path = build_path("/messages", &query);Also applies to: 111-116, 256-261
🤖 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/client.rs` around lines 91 - 96, Extract the repeated path-and-query construction from send, list_messages, and list_inbox_messages into a shared helper function that accepts a base path and query string, returning the base path unchanged for an empty query or appending ?query otherwise. Replace each duplicated conditional with calls to this helper.src/types/settings.rs (1)
206-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
is_valid_time_formatandparse_timehave redundant validation layers.
validate_work_hourscallsis_valid_time_format(which checks format and digit positions), thenparse_time(which re-splits and parses), then range-checks the parsed values. The flow is correct but the two functions overlap in responsibility.is_valid_time_formatcould return parsed(u8, u8)directly, eliminatingparse_timeand the redundant-1sentinel returns.♻️ Proposed refactor
-fn is_valid_time_format(s: &str) -> bool { - s.len() == 5 - && s.as_bytes().iter().enumerate().all(|(i, &b)| match i { - 0 | 1 => b.is_ascii_digit(), - 2 => b == b':', - 3 | 4 => b.is_ascii_digit(), - _ => false, - }) -} - -fn parse_time(value: &str) -> (i32, i32) { - let parts: Vec<&str> = value.split(':').collect(); - if parts.len() != 2 { - return (-1, -1); - } - let h: i32 = parts[0].parse().unwrap_or(-1); - let m: i32 = parts[1].parse().unwrap_or(-1); - (h, m) +fn parse_time(s: &str) -> Option<(u8, u8)> { + let b = s.as_bytes(); + if s.len() == 5 + && b[0].is_ascii_digit() + && b[1].is_ascii_digit() + && b[2] == b':' + && b[3].is_ascii_digit() + && b[4].is_ascii_digit() + { + let h = (b[0] - b'0') * 10 + (b[1] - b'0'); + let m = (b[3] - b'0') * 10 + (b[4] - b'0'); + if h <= 23 && m <= 59 { + return Some((h, m)); + } + } + None }Then update
validate_work_hoursto use the single function:- if !is_valid_time_format(start) { - return Err(crate::Error::Validation(format!( - "workHoursStart must be in HH:mm format, got \"{}\"", - start - ))); - } - if !is_valid_time_format(end) { - return Err(crate::Error::Validation(format!( - "workHoursEnd must be in HH:mm format, got \"{}\"", - end - ))); - } - - let (h1, m1) = parse_time(start); - let (h2, m2) = parse_time(end); - - if !(0..=23).contains(&h1) || !(0..=59).contains(&m1) { - return Err(crate::Error::Validation(format!( - "workHoursStart \"{}\" contains invalid time values", - start - ))); - } - if !(0..=23).contains(&h2) || !(0..=59).contains(&m2) { - return Err(crate::Error::Validation(format!( - "workHoursEnd \"{}\" contains invalid time values", - end - ))); - } + let (h1, m1) = parse_time(start).ok_or_else(|| { + crate::Error::Validation(format!( + "workHoursStart must be in HH:mm format with valid values, got \"{}\"", + start + )) + })?; + let (h2, m2) = parse_time(end).ok_or_else(|| { + crate::Error::Validation(format!( + "workHoursEnd must be in HH:mm format with valid values, got \"{}\"", + end + )) + })?; + let _ = (h1, m1, h2, m2);🤖 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/types/settings.rs` around lines 206 - 224, Refactor is_valid_time_format to validate the HH:MM structure and return parsed hour/minute values, such as Option<(u8, u8)>, instead of a boolean. Remove parse_time and its (-1, -1) sentinel handling, then update validate_work_hours to consume the parsed tuple and retain the existing range checks.tests/types_test.rs (1)
1-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for settings serialization and key validation paths.
The test suite covers enum serde and message validation well, but has notable gaps:
- No
SettingsMessagesorDeviceSettingsserialization roundtrip test — this would have caught thesnake_casevscamelCaseinconsistency flagged insrc/types/settings.rs.- No test for
Message::validatewith pastschedule_at— the future-check on line 159-164 ofmessage.rsis untested.- No test for
MessageState::validatewith invalid state keys — only the valid case (line 137-152) is tested.- No test for
ClientConfig::validatewith partial basic auth — e.g., username without password.Consider adding:
🧪 Suggested additional tests
#[test] fn test_settings_messages_serde_roundtrip() { let settings = SettingsMessages { send_interval_min: Some(1), send_interval_max: Some(10), limit_period: Some(LimitPeriod::PerHour), limit_value: Some(100), sim_selection_mode: Some(SimSelectionMode::RoundRobin), log_lifetime_days: Some(30), processing_order: Some(MessagesProcessingOrder::Fifo), work_hours_enabled: Some(true), work_hours_start: Some("09:00".into()), work_hours_end: Some("17:00".into()), }; let json = serde_json::to_string(&settings).unwrap(); // Verify camelCase serialization assert!(json.contains("sendIntervalMin")); assert!(json.contains("sendIntervalMax")); assert!(json.contains("limitPeriod")); let deserialized: SettingsMessages = serde_json::from_str(&json).unwrap(); assert_eq!(deserialized.send_interval_min, Some(1)); } #[test] fn test_message_validate_schedule_at_in_past() { let msg = Message { message: Some("hi".into()), phone_numbers: vec!["123".into()], schedule_at: Some(chrono::Utc::now()), ..Default::default() }; assert!(msg.validate().is_err()); } #[test] fn test_message_state_validate_invalid_state_key() { use std::collections::HashMap; let mut states = HashMap::new(); states.insert("InvalidState".to_string(), chrono::Utc::now()); let state = MessageState { id: "test".into(), device_id: "dev".into(), state: ProcessingState::Pending, is_hashed: false, is_encrypted: false, recipients: vec![], states, text_message: None, data_message: None, hashed_message: None, }; assert!(state.validate().is_err()); }🤖 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 `@tests/types_test.rs` around lines 1 - 294, Add focused tests in the existing types test suite for the uncovered validation and serialization paths: round-trip SettingsMessages and assert camelCase keys, including representative fields such as send_interval_min; add a Message::validate test with schedule_at set in the past; add a MessageState::validate test containing an invalid states key; and add a ClientConfig::validate test for partial basic authentication, such as a username without a password. Reuse the existing enum and struct construction patterns and assert each case returns an error where appropriate.src/types/device.rs (1)
20-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
#[serde(rename_all = "camelCase")]onSimCardfor consistency.
SimCarduses individual#[serde(rename = "...")]attributes forslot_indexandsim_number, whileDeviceand other structs in the crate use#[serde(rename_all = "camelCase")]. Applying the container-level attribute is cleaner and consistent with the rest of the codebase.♻️ Proposed refactor
#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] pub struct SimCard { - #[serde(rename = "slotIndex")] pub slot_index: i32, - #[serde(rename = "simNumber")] pub sim_number: i32, #[serde(default, skip_serializing_if = "Option::is_none")] pub phone_number: Option<String>, #[serde(default, skip_serializing_if = "Option::is_none")] pub carrier_name: Option<String>, + #[serde(default, skip_serializing_if = "Option::is_none")] pub iccid: Option<String>, }🤖 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/types/device.rs` around lines 20 - 31, Replace the individual serde rename attributes on the SimCard struct with a container-level #[serde(rename_all = "camelCase")] attribute, preserving the existing optional-field serialization behavior and field names.src/types/inbox.rs (1)
5-15: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a catch-all variant for
IncomingMessageType.If the API adds a new incoming message type in the future, deserialization of
IncomingMessagewill fail entirely. Adding an#[serde(other)]variant (e.g.,Other) would improve forward compatibility.♻️ Proposed refactor
pub enum IncomingMessageType { #[serde(rename = "SMS")] Sms, #[serde(rename = "DATA_SMS")] DataSms, #[serde(rename = "MMS")] Mms, #[serde(rename = "MMS_DOWNLOADED")] MmsDownloaded, + #[serde(other)] + Other, }🤖 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/types/inbox.rs` around lines 5 - 15, Update the IncomingMessageType enum to include a catch-all variant annotated with #[serde(other)], such as Other, so unknown API message types deserialize successfully while preserving the existing named variants.src/types/health.rs (1)
18-26: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a catch-all variant for
HealthStatus.If the API introduces a new status value (e.g.,
"error"or"degraded"), deserialization ofHealthStatuswill fail and the entireHealthResponseparse will error out. Adding an#[serde(other)]variant (e.g.,Unknown) improves forward compatibility.♻️ Proposed refactor
pub enum HealthStatus { #[serde(rename = "pass")] Pass, #[serde(rename = "warn")] Warn, #[serde(rename = "fail")] Fail, + #[serde(other)] + Unknown, }🤖 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/types/health.rs` around lines 18 - 26, Add a catch-all `Unknown` variant to the `HealthStatus` enum with `#[serde(other)]` so unrecognized API status values deserialize safely without failing the containing `HealthResponse`.src/types/mod.rs (1)
13-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider scoped re-exports instead of glob re-exports.
Glob re-exports (
pub use X::*) from all 11 submodules risk name collisions as the crate grows (e.g., if two modules define types with the same name). Prefer explicit re-exports of the public API surface, or at minimum document the collision risk.🤖 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/types/mod.rs` around lines 13 - 23, Replace the glob re-exports in the types module with explicit re-exports of the intended public types from each submodule, using the declarations in auth, device, health, inbox, log, message, options, responses, settings, upstream, and webhook; avoid exposing every item and prevent future name collisions.src/encryption.rs (2)
140-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer library-provided PKCS7 padding over manual implementation.
The
ciphercrate (already a dependency at 0.4.0) providescipher::block_padding::Pkcs7which handles padding and unpadding safely, including constant-time validation. Using it eliminates the manualpkcs7_pad/pkcs7_unpadfunctions and their associated edge-case risk.🤖 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/encryption.rs` around lines 140 - 162, Replace the manual pkcs7_pad and pkcs7_unpad implementations with cipher::block_padding::Pkcs7, updating encryption and decryption call sites to use the library’s padding and unpadding APIs. Remove the helper functions and add any required cipher trait imports while preserving existing error handling.
56-56: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftPBKDF2-SHA1 is deprecated; prefer SHA-256 if compatibility allows.
SHA-1 is no longer recommended for cryptographic use. PBKDF2-SHA256 provides better security margin with no performance penalty. If the
$aes-256-cbc/pbkdf2-sha1format is a hard compatibility constraint, document the rationale; otherwise, migrate topbkdf2_hmac::<sha2::Sha256>.Also applies to: 117-117
🤖 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/encryption.rs` at line 56, Replace the PBKDF2 SHA-1 invocations in the encryption routines with pbkdf2_hmac::<sha2::Sha256>, updating imports and ensuring encryption and decryption use the same derivation. If the existing $aes-256-cbc/pbkdf2-sha1 format must remain compatible, preserve it explicitly and document the rationale.src/webhook.rs (1)
36-42: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid
format!allocation and useMac::verify_slicefor comparison.Calling
mac.update(body.as_bytes())thenmac.update(timestamp.as_bytes())is equivalent to updating with the concatenated message, avoiding a heap allocation. Decoding the hex signature and callingmac.verify_sliceeliminates the customconstant_time_eqfunction entirely and uses the library's built-in constant-time comparison.♻️ Proposed refactor: eliminate allocation and custom comparison
pub fn verify_signature(secret_key: &str, body: &str, timestamp: &str, signature: &str) -> bool { let mut mac = match Hmac::<Sha256>::new_from_slice(secret_key.as_bytes()) { Ok(m) => m, Err(_) => return false, }; - let message = format!("{}{}", body, timestamp); - mac.update(message.as_bytes()); - - let expected_bytes = mac.finalize().into_bytes(); - let expected_hex = hex::encode(expected_bytes); - - constant_time_eq(expected_hex.as_bytes(), signature.as_bytes()) + mac.update(body.as_bytes()); + mac.update(timestamp.as_bytes()); + + match hex::decode(signature) { + Ok(sig_bytes) => mac.verify_slice(&sig_bytes).is_ok(), + Err(_) => false, + } } - -fn constant_time_eq(a: &[u8], b: &[u8]) -> bool { - if a.len() != b.len() { - return false; - } - let result = a - .iter() - .zip(b.iter()) - .fold(0u8, |acc, (x, y)| acc | (x ^ y)); - result == 0 -}🤖 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/webhook.rs` around lines 36 - 42, In the webhook signature verification logic, avoid constructing the concatenated message with format!. Update the MAC separately with body.as_bytes() and timestamp.as_bytes(), decode the provided hexadecimal signature, and use Mac::verify_slice for constant-time verification; remove the expected_hex generation and custom constant_time_eq usage..github/workflows/release.yml (1)
14-16: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider trusted publishing and add a pre-publish test step.
cargo publishwith a long-livedCARGO_REGISTRY_TOKENis more vulnerable to token leakage than OIDC-based trusted publishing, which crates.io supports natively. Additionally, the workflow publishes without running tests — a broken tag push could publish a broken release.♻️ Suggested improvements
- uses: actions-rust-lang/setup-rust-toolchain@v1 + with: + cache: false + - run: cargo test --all-features - run: cargo publish - env: - CARGO_REGISTRY_TOKEN: ${{ secrets.CARGO_REGISTRY_TOKEN }} + env: + CARGO_REGISTRY_TOKEN: ${{ secrets.CARGO_REGISTRY_TOKEN }}For trusted publishing, replace the token with OIDC federation per the crates.io trusted publishing guide.
🤖 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/release.yml around lines 14 - 16, Update the release workflow’s cargo publish step to use crates.io OIDC trusted publishing instead of the long-lived CARGO_REGISTRY_TOKEN secret, configuring the required permissions and authentication setup. Add a pre-publish cargo test step so the release is published only after tests pass.Source: Linters/SAST tools
🤖 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:
- Around line 9-16: Add a workflow-level permissions block granting only read
access needed by the CI jobs, such as contents: read, and set
persist-credentials: false on every actions/checkout@v4 step in the lint, test,
build, and doc jobs.
In `@Cargo.toml`:
- Line 27: Make the cipher dependency optional in Cargo.toml and add its
dependency entry to the encryption feature so it is enabled only when encryption
is selected; verify encryption-related code still resolves with the feature
enabled.
In `@Makefile`:
- Line 14: Make the coverage target in the Makefile meaningful: either remove
coverage entirely, or implement it using the repository’s configured coverage
tool such as cargo-llvm-cov, ensuring make coverage runs tests and generates a
coverage report instead of only invoking test.
In `@README.md`:
- Line 253: Update the README license URL reference to use the repository’s
default `master` branch instead of `main`, preserving the existing LICENSE path.
In `@src/client.rs`:
- Line 134: URL-encode every interpolated path parameter before constructing
request paths. Update the path-building code around the message, device,
webhook, and auth-token operations (including the formats at the referenced
locations) to encode each ID as a single URL path segment, preserving slashes,
query markers, fragments, and spaces as data.
- Around line 123-127: Update the total-count parsing in the relevant
response-handling code, including both occurrences of the `X-Total-Count` logic,
so missing or invalid headers do not silently use the current page length.
Prefer representing an unavailable total explicitly with `None` (and updating
the return type and callers), or use the agreed explicit sentinel such as `0`;
document the chosen behavior.
- Line 283: Encode the RFC3339 timestamp query parameters in the URL
construction within the logs request, ensuring both from and to values are
percent-encoded before interpolation. Alternatively, format the UTC timestamps
with chrono’s SecondsFormat::Secs and a Z suffix to avoid the unencoded plus
sign.
In `@src/config.rs`:
- Around line 87-94: Update Config::validate to reject partial basic
authentication: allow username and password only when both are present, allow
token-based authentication, and continue rejecting configurations with no
credentials. Return InvalidConfig with a clear message for a username/password
mismatch, and ensure Client::new receives only complete basic-auth credentials.
In `@src/encryption.rs`:
- Around line 44-49: Validate the iterations argument in Encryption’s
with_iterations constructor, rejecting zero and values below the project’s safe
minimum before constructing Self; use an assertion, panic, or fallible API
consistent with existing crate conventions, and ensure invalid counts cannot
reach PBKDF2.
- Around line 69-74: Replace the raw-pointer `unsafe` block in the CBC
processing logic with `buf.chunks_exact_mut(16)`, converting each chunk via
`GenericArray::from_mut_slice`; apply the same change to both affected
block-processing sections while preserving the existing per-block
encryption/decryption flow.
In `@src/http.rs`:
- Around line 136-151: Extend map_error to explicitly handle common HTTP
statuses 401, 403, 404, 422, and 429 instead of routing them through the generic
Error::Client case. Add corresponding Error variants if the enum supports them,
update all required constructors and matches, and preserve the existing body
details so callers can distinguish these errors programmatically.
- Around line 103-134: Configure a finite default timeout when constructing the
default reqwest client in Client::new, while preserving any explicitly supplied
client from ClientConfig; verify execute_with_auth uses that client for all
requests so unresponsive servers cannot hang indefinitely.
In `@src/lib.rs`:
- Line 28: Remove the `reqwest/blocking` feature from the Cargo.toml dependency
configuration, and remove the `blocking` entry from the crate documentation
table in the module-level docs of `src/lib.rs` until a synchronous client API is
implemented.
In `@src/types/message.rs`:
- Around line 90-92: Message::validate() does not enforce the documented 1–3
range for sim_number. Add validation for Some(sim_number) to reject values below
1 or above 3, returning the same validation error type and style used for other
invalid Message fields.
In `@src/types/options.rs`:
- Around line 101-108: Replace the silent unwrap_or_default() fallbacks in
ListInboxOptions and ListMessagesOptions query-parameter serialization with
explicit error propagation or expect(...) messages; update both message_type
serialization sites to ensure serialization failures cannot produce empty query
values.
- Around line 204-213: Extend ListMessagesOptions::validate() and
ListInboxOptions::validate() to reject invalid pagination values: enforce the
documented 1–100 range for limit and ensure offset is non-negative. Return an
appropriate crate::Error::Validation with clear messages before existing
date-range validation succeeds.
- Around line 98-127: Implement a validate() method for ListInboxOptions,
matching the date-range validation used by ListMessagesOptions: accept missing
bounds, but reject configurations where from is later than to with the
established validation error type/message. Ensure the validation method is
exposed consistently with the existing options validation API and can be invoked
before to_query_params().
In `@src/types/settings.rs`:
- Around line 58-65: Call DeviceSettings::validate() in the client methods
replace_settings and update_settings before serializing or sending the settings;
propagate validation failures with ?. Ensure both request paths validate the
supplied DeviceSettings so invalid values never reach the HTTP request.
- Around line 63-204: Add #[serde(rename_all = "camelCase")] to the
SettingsPing, SettingsLogs, SettingsWebhooks, SettingsGateway, and
SettingsReceiver structs, matching the existing SettingsMessages serialization
behavior so all nested multi-word fields use camelCase.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 14-16: Update the release workflow’s cargo publish step to use
crates.io OIDC trusted publishing instead of the long-lived CARGO_REGISTRY_TOKEN
secret, configuring the required permissions and authentication setup. Add a
pre-publish cargo test step so the release is published only after tests pass.
In `@Cargo.toml`:
- Line 26: Remove the unused optional tokio dependency declaration from
Cargo.toml, leaving the dev-dependency declaration intact since tokio is only
used for development and no feature activates the optional entry.
In `@src/client.rs`:
- Around line 91-96: Extract the repeated path-and-query construction from send,
list_messages, and list_inbox_messages into a shared helper function that
accepts a base path and query string, returning the base path unchanged for an
empty query or appending ?query otherwise. Replace each duplicated conditional
with calls to this helper.
In `@src/encryption.rs`:
- Around line 140-162: Replace the manual pkcs7_pad and pkcs7_unpad
implementations with cipher::block_padding::Pkcs7, updating encryption and
decryption call sites to use the library’s padding and unpadding APIs. Remove
the helper functions and add any required cipher trait imports while preserving
existing error handling.
- Line 56: Replace the PBKDF2 SHA-1 invocations in the encryption routines with
pbkdf2_hmac::<sha2::Sha256>, updating imports and ensuring encryption and
decryption use the same derivation. If the existing $aes-256-cbc/pbkdf2-sha1
format must remain compatible, preserve it explicitly and document the
rationale.
In `@src/types/device.rs`:
- Around line 20-31: Replace the individual serde rename attributes on the
SimCard struct with a container-level #[serde(rename_all = "camelCase")]
attribute, preserving the existing optional-field serialization behavior and
field names.
In `@src/types/health.rs`:
- Around line 18-26: Add a catch-all `Unknown` variant to the `HealthStatus`
enum with `#[serde(other)]` so unrecognized API status values deserialize safely
without failing the containing `HealthResponse`.
In `@src/types/inbox.rs`:
- Around line 5-15: Update the IncomingMessageType enum to include a catch-all
variant annotated with #[serde(other)], such as Other, so unknown API message
types deserialize successfully while preserving the existing named variants.
In `@src/types/mod.rs`:
- Around line 13-23: Replace the glob re-exports in the types module with
explicit re-exports of the intended public types from each submodule, using the
declarations in auth, device, health, inbox, log, message, options, responses,
settings, upstream, and webhook; avoid exposing every item and prevent future
name collisions.
In `@src/types/settings.rs`:
- Around line 206-224: Refactor is_valid_time_format to validate the HH:MM
structure and return parsed hour/minute values, such as Option<(u8, u8)>,
instead of a boolean. Remove parse_time and its (-1, -1) sentinel handling, then
update validate_work_hours to consume the parsed tuple and retain the existing
range checks.
In `@src/webhook.rs`:
- Around line 36-42: In the webhook signature verification logic, avoid
constructing the concatenated message with format!. Update the MAC separately
with body.as_bytes() and timestamp.as_bytes(), decode the provided hexadecimal
signature, and use Mac::verify_slice for constant-time verification; remove the
expected_hex generation and custom constant_time_eq usage.
In `@tests/types_test.rs`:
- Around line 1-294: Add focused tests in the existing types test suite for the
uncovered validation and serialization paths: round-trip SettingsMessages and
assert camelCase keys, including representative fields such as
send_interval_min; add a Message::validate test with schedule_at set in the
past; add a MessageState::validate test containing an invalid states key; and
add a ClientConfig::validate test for partial basic authentication, such as a
username without a password. Reuse the existing enum and struct construction
patterns and assert each case returns an error where appropriate.
🪄 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: b0052a5b-8d7a-4658-8282-23cd1187058f
📒 Files selected for processing (25)
.github/workflows/ci.yml.github/workflows/release.ymlCargo.tomlMakefileREADME.mdsrc/client.rssrc/config.rssrc/encryption.rssrc/error.rssrc/http.rssrc/lib.rssrc/types/auth.rssrc/types/device.rssrc/types/health.rssrc/types/inbox.rssrc/types/log.rssrc/types/message.rssrc/types/mod.rssrc/types/options.rssrc/types/responses.rssrc/types/settings.rssrc/types/upstream.rssrc/types/webhook.rssrc/webhook.rstests/types_test.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client.rs (1)
111-129: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winClient methods skip available input validation before sending requests.
replace_settingsandupdate_settingscallsettings.validate()?before their API calls, butlist_messages,list_inbox_messages, andget_logsdo not validate their inputs despite validation logic being available. Invalid date ranges, pagination bounds, and limit values are silently sent to the server.
src/client.rs#L111-L129: Addoptions.validate()?at the start oflist_messagesbeforeoptions.to_url_query().src/client.rs#L255-L273: Addoptions.validate()?at the start oflist_inbox_messagesbeforeoptions.to_url_query().src/client.rs#L276-L289: Add afrom > tocheck returningError::Validationbefore constructing the logs URL.🤖 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/client.rs` around lines 111 - 129, Add options.validate()? at the start of list_messages and list_inbox_messages, before calling to_url_query. In get_logs, reject ranges where from is later than to by returning Error::Validation before constructing the request URL; apply these changes at src/client.rs lines 111-129, 255-273, and 276-289.
🤖 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 `@src/types/options.rs`:
- Around line 67-76: Update ListInboxOptions::validate() to validate limit is
within 1–100 and offset is non-negative, matching
ListMessagesOptions::validate(). Preserve the existing from/to date-range
validation and return the established validation errors for invalid pagination
values.
---
Nitpick comments:
In `@src/client.rs`:
- Around line 111-129: Add options.validate()? at the start of list_messages and
list_inbox_messages, before calling to_url_query. In get_logs, reject ranges
where from is later than to by returning Error::Validation before constructing
the request URL; apply these changes at src/client.rs lines 111-129, 255-273,
and 276-289.
🪄 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: c6f056e1-399c-4d07-8a7b-9562f059a68d
📒 Files selected for processing (18)
.github/workflows/ci.yml.github/workflows/release.ymlCargo.tomlMakefileREADME.mdsrc/client.rssrc/config.rssrc/encryption.rssrc/error.rssrc/http.rssrc/lib.rssrc/types/device.rssrc/types/health.rssrc/types/inbox.rssrc/types/options.rssrc/types/settings.rssrc/webhook.rstests/types_test.rs
💤 Files with no reviewable changes (1)
- src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/workflows/release.yml
- src/types/device.rs
- .github/workflows/ci.yml
- src/types/health.rs
- Makefile
- src/types/inbox.rs
- src/config.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit